Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature] Allow async predicates for RunAndWaitFor methods #2576

Open
maw136 opened this issue May 12, 2023 · 11 comments
Open

[Feature] Allow async predicates for RunAndWaitFor methods #2576

maw136 opened this issue May 12, 2023 · 11 comments

Comments

@maw136
Copy link
Contributor

maw136 commented May 12, 2023

Given RequestFinished events already contain Response object it would be more flexible for the Predicate<T> that is passed by options of RunAndWaitForRequestFinished (and WaitForRequestFinished) methods to use the IReponse object instead of IRequest.
I can make the changes in code-base but I'd like to discuss architecture here - before committing to these changes.

@maw136
Copy link
Contributor Author

maw136 commented May 13, 2023

I have created a PR for this: #2577
It is a code breaking change so I strongly request comments on the issue.

@mxschmitt
Copy link
Member

We strongly try to avoid breaking changes.

Would calling await request.ResponseAsync() work for you to get the response?

Could you share code which demonstrates the issue you are trying to solve?

@maw136
Copy link
Contributor Author

maw136 commented May 14, 2023

Let's assume this kind of snippet:

 PageRunAndWaitForRequestFinishedOptions options = new()
            {
                Predicate = finishedRequest => responseTemplate.Matches(finishedRequest.Url, new HttpMethod(finishedRequest.Method)),
                Timeout = (float?)timeout?.TotalMilliseconds
            };

The Predicate and similar Request/Response matchers assume synchronous context so there is no proper way to use async inside them.
What's more, RequestFinished event always happens after Response so, there should always be a valid Response there, or am I missing something?

@mxschmitt
Copy link
Member

The more high level question which I wanted to ask was, what data from the Response object do you try to access in PageRunAndWaitForRequestFinished? In this example its just Request.Url and Request.Method which seems already working fine for your use-case?

@maw136
Copy link
Contributor Author

maw136 commented May 15, 2023

I haven't been precise enough. We need to additionally access StatusCode in the Predicate.

@maw136
Copy link
Contributor Author

maw136 commented May 15, 2023

So I've created an additional PR for possibly less breaking effect:
#2578

@maw136
Copy link
Contributor Author

maw136 commented May 16, 2023

But I don't like that solution, even for compatibilities sake.

@mxschmitt
Copy link
Member

Instead of coming up with new APIs or PRs, lets first try to understand the actual problem better. Could you please share more information what you are trying to do? For what kind of network requests do you wait for, what triggers it, is it a normal navigation or after some page interaction like click?

Would RunAndWaitForResponseAsync work for you with an additional await response.FinishedAsync() at the end?

Would it be possible to share a minimal reproduction which shows whats not working?

@maw136
Copy link
Contributor Author

maw136 commented May 29, 2023

The requests/responses are triggered by a normal application operation. The app is somewhat talkative, and it does some async requests for new data.
We would like to have access to IResponse on RequestFinished event because as the docs say that event is fired after the body of Response had been loaded. We use the (RunAnd)WaitFor... APIs to wait for our application to process stuff. It doesn't work sometimes, when Angular has more data to parse and refresh DOM the flow breaks. We are actively working on a better approach, like waiting for Locators and such. But some of the crucial functionality depends on those Request/Response pairs.
I switched back our uses to ...ForResponse... from ...ForRequestFinished... as it's been like that before.
The repro is so exceedingly difficult to achieve to be honest, even in perfect conditions. There are reproducible problems with producing the errors such as having a breakpoint near the failing line makes the errors go away.
We have a usecase to acquire multiple IResponse objects after Wait. An API to ...WaitForMultiple... would be the best for us.
I could introduce such API with your guidance.
To be honest I prefer making lots of PRs to see 'what sticks', but I understand that it may not be a good flow for everyone, what I mean is, I explain myself (and our team needs) in code/PR better than in just a natural language text.
The other issue with ...RequestFinished... is that the filtering predicate is not async co it can't access the Response of the Request, even that it is finished, but that is strictly connected to the issue of waiting as long as possible as a patch to the race condition that happens. We are waiting for reponses/parsing of responses hoping that the Angular will be fast enough with DOM update.
Either way IMHO Response object should be accessible at RequestFinished predicate despite our app.
As for click interaction it is also in our flow. Some Clicks trigger multiple responses that we would like to aquire body of - after the predicate filters them out.

I hope it is intelligible. Awaiting all the questions and suggestions. I'd like to contribute good code :)

@mxschmitt
Copy link
Member

Sorry for the late response, was out of office.

We are actively working on a better approach, like waiting for Locators and such. But some of the crucial functionality depends on those Request/Response pairs.

This sounds like web-first assertions should just work for you.

is that the filtering predicate is not async co it can't access the Response of the Request, even that it is finished

I was looking at Node.js, and how we do it there. Turns out, there we have async predicate support.

So we would be more than happy to make the predicate async as well in the RunAndWaitFor methods, e.g. here.

Are you fine if I change this issue title accordingly to that?

@maw136
Copy link
Contributor Author

maw136 commented Jun 19, 2023

Of course. Be my guest.

@mxschmitt mxschmitt changed the title [Feature] Enhance processing of RequestFinished events [Feature] Allow async predicates for RunAndWaitFor methods Jun 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants