Allow awaiting futures automatically in parameters

[blastrock]

Async fixture are kind of hard to use currently as reported in #157 . This PR adds a syntax to make tests with async fixtures easier to write.

Before:

    #[rstest]
    async fn it_can_return_its_ip(#[future] client: Client) {
        let client = client.await;
        client.doThis();
        client.doThat();
    }

After:

    #[rstest]
    async fn it_can_return_its_ip(#[await_future] client: Client) {
        client.doThis();
        client.doThat();
    }

This PR is ready code-wise, but it still lacks some tests and documentation. I wanted to get feedback for this approach before going further, please tell me what you think :)

[la10736]

First of all thanks too much for your effort and ... WOW it's a lot of work!!!!

Please be patient.... I've not too much time to look into it. I hope to find some times in the next days (1 or 2 weeks ... sorry :()

[blastrock]

No problem, I know what it is to manage an open source project, no pressure :)

Initially I just did it for fun and to discover how macro worked in Rust, then it become something more serious ^^

[la10736]

Sorry, maybe I've lost the big plan... What's the real difference between #[future]and#[future_awit]? Just add the await call on fixture resolution?
In this case I guess that the code cold be really simplified and I handle this case as a special case of the first one. (I'm writing this by mobile)

Anyway... follow my review of this code.

Great and huge work!!! But we should fix some things to this draft

1. Rebase it to main
2. Avoid to mix render and parse: parse should add all information to enable render to write the correct code
3. Use Resolver composability instead of add new field to ArgumentResolver struct
4. Add tests for catching error in parsing
5. Find a way to don't spread async_args in all function signature

Thanks again for your effort. I'll try to support your work by doing my best and give you a fast (I hope) feedback.

[la10736]

What about extract this check in a function and use matches!()?

[la10736]

I'm quite sure that we can avoid to spread await_args in all functions signature... I think that test and attributes can contains all information. I we start to add a new argument for each feature the code will become a mess (more than it already is :) ).

[blastrock]

Thanks for the review! I'm a bit busy too, I'll try to apply the changes this weekend.

Sorry, maybe I've lost the big plan... What's the real difference between #[future]and#[future_awit]? Just add the await call on fixture resolution?

Yes, that's it, just the await call. The rationale is that if I need to write

    #[rstest]
    async fn it_can_return_its_ip(#[future] client: Client) {
        let client = client.await;
        client.doThis();
        client.doThat();
    }

I might as well write

    #[rstest]
    async fn it_can_return_its_ip() {
        let client = make_client().await;
        client.doThis();
        client.doThat();
    }

and not use rstest at all, it's the same number of lines.

In this case I guess that the code cold be really simplified and I handle this case as a special case of the first one. (I'm writing this by mobile)

I can't remember exactly but I couldn't reuse the future code. The future attr is handled by just replacing the type T with impl Future<T> . In the case of await we need to generate the await code in the wrapper code of fixtures and test cases.

[la10736]

OOOHHHHH!!! You're right ... also the code that handle future is a mess 😄

Ok, we can start to include your version and after refactor them. In parsing stage we should just identify the attributes,
remove them and accumulate the info about what arguments need impl Future modification and what need await.
On render stage use these information to modify type and resolver.

[blastrock]

Ok, I managed to work something out! I need some feedback though.

As usual, no hurry :) When you have some time, I'd like you to check the commit 51d626c . This is something I needed to use a resolver to add the await expression. I moved the decision to use a fixture in the parsing stage instead of the end of the injection stage. There is one downside to this, it's that the extract_fixtures call must be first (I put a comment there so that you notice it), I know this is something you didn't like in my first patch but I couldn't find a better idea, tell me if you have one.

Then, you can just review the AwaitResolver part in the "--wip--" commit and tell me if it is the approach you were thinking of.

Also note that all integration tests pass, but I broke a lot of unit tests, so no need to review in too much detail, this is far from the final version. And I didn't apply all the changes you requested yet ^^

[la10736]

Ok, I spend all the morning to try to recover from my mind what I was doing in my code 😄. I also started to write something new from scratch....
We need to introduce something new here: all resolvable variable should have an Option like

#[derive(PartialEq, Debug, Clone)]
pub(crate) enum AsyncType {
    Plain(Type),
    Await(Type),
}

Ok, it's simple to add it to Fixture struct but we need it also in case and values that use plain Ident for now 😢

We need an exit strategy: Forget all my comments about don't mix parser and render phases and also for resolver part. I'll merge your PR when you'll fix all comments and later I'll open a ticket for refactor all code and remove this mess.

I'm sorry .... we're paying an hack that I did to introduce #[future] attribute as fast as possible.

[blastrock]

Here it is. I kept the last version with the resolvers, fixed all the tests and addressed your comments. I didn't write new unit tests since you're going to refactor and they'll probably be more pain to refactor.

We need to introduce something new here: all resolvable variable should have an Option like

That's an option, but it will change a lot of code, much like my first solution. I let you experiment with that and be the judge ^^

Tell me if you need some more changes before merging :)

Also, if I can add one more controversial opinion: I know I'm the one who chose it but await_future is kind of a long name. I initially wanted to just write #[await] but you can't put a keyword in there. Maybe we should go with an abbreviation #[awt]?

[la10736]

Also, if I can add one more controversial opinion: I know I'm the one who chose it but await_future is kind of a long name. I initially wanted to just write #[await] but you can't put a keyword in there. Maybe we should go with an abbreviation #[awt]?

Also I don't like this name and I don't like that is another kind of #[future] ... so what about #[future(awt)]? ... ok is longer but don't introduce a new keyword but just an option on an existing one.

[pleshevskiy]

I just started using your library for integration tests and I absolutely do not understand for what cases we need to always manually await for async fixtures? Can't we automatically await for them in the order we specified in test_case function?

[pleshevskiy]

At worst, I would prefer a single modifier before the test_case function instead of writing #[await_future] / #[future(awt)] each time

For example:

#[rstest]
#[await_futures]
async fn it_can_return_its_ip(
    #[future] di_container: RootModule,
    #[future] client: Client,
) {
    let foo: Box<dyn FooPort> = di_container.provide().unwrap();
    client.doThis();
    client.doThat();
    foo.execute();
}

[la10736]

There are a lot of cases where the code that you should test take a non awaited future bu just a future: fixture can provide this kind of references and #[future] is something that help you on remove the boiler plate on receiving a Future. If you don't need a future from your fixture why don't create a concrete (await for the future) and return it like a standard fixture?

Ok... #[awt] is good

[pleshevskiy]

Or maybe we're approaching this problem from the wrong side. What if we add a macro for fixture to do a wrapper over the async function and create a convenient wrapper for us?

#[fixture]
#[block]
pub async fn di_container() -> DiContainer {
    let db_pool = PgConnectionPool::create(CreateDbConnectionPoolParams {
        host: "localhost",
        port: 6666,
        user: "test",
        password: "test",
        db_name: "test",
    })
    .await;

    create_di_container(db_pool)
}


#[fixture]
#[block]
pub async fn client(di_container: DiContainer) -> TestClient {
    let app = create_app(CreateAppParams {
        di_container: Arc::new(di_container.await),
    });

    TestClient::new(app)
}

[pleshevskiy]

I think this approach would also solve issue #141

[la10736]

I close this PR in favor to #186
Thank very much for your effort!!!!

[blastrock]

Thank you for your work on this! <3