This repository has been archived by the owner on Jan 30, 2025. It is now read-only.
Async waitForNavigation #467
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Jul 21, 2022
Closed
inancgumus
force-pushed
the
feat/440-async-waitfornavigation
branch
4 times, most recently
from
61d50a4 to
39fbbf0
Compare
imiric
reviewed
Jul 25, 2022
imiric
added
the
breaking
imiric
suggested changes
Jul 28, 2022
|
LGTM. @imiric you raise good points on the tests. I also think we should be checking that |
The callback returned from RegisterCallback should not be run as a goroutine. Since its job is to carry a task to the event loop, the task should be done in a goroutine first, and then let the task call the callback within the goroutine.
Since waitForNavigation is async now.
Since the Click and WaitForNavigation are now async, we don't need to call each in separate goroutines. Instead, we call them in the event loop, and the event loop will take care of async stuff for us.
ankur22
force-pushed
the
feat/440-async-waitfornavigation
branch
from
39fbbf0 to
41ae327
Compare
ankur22
added a commit
that referenced
this pull request
Aug 8, 2022
We can now use Promise.all when we interact with an element that causes a navigation to occur, and so we need to also use waitForNavigation before carrying on to the next steps in the tests. This helps prevent race conditions between clicking and waiting. Resolves: #467 (comment)
We can now use Promise.all when we interact with an element that causes a navigation to occur, and so we need to also use waitForNavigation before carrying on to the next steps in the tests. This helps prevent race conditions between clicking and waiting. Resolves: #467 (comment)
ankur22
force-pushed
the
feat/440-async-waitfornavigation
branch
from
8a9f052 to
886eda5
Compare
ankur22
added a commit
that referenced
this pull request
Aug 8, 2022
The test now checks the state of the promise before passing the test. Resolves: #467 (comment)
ankur22
added a commit
that referenced
this pull request
Aug 8, 2022
The test now checks the state of the promise before passing the test. Resolves: #467 (comment)
ankur22
force-pushed
the
feat/440-async-waitfornavigation
branch
from
d37be91 to
a68a949
Compare
The test now checks the state of the promise before passing the test. Resolves: #467 (comment)
ankur22
force-pushed
the
feat/440-async-waitfornavigation
branch
from
a68a949 to
331465f
Compare
ankur22
added a commit
that referenced
this pull request
Aug 9, 2022
We just want to make sure that the promises actually complete and are fulfilled before we end the test, so we're checking the state of the promise in `tb.await` as well as after `tb.await`. There's a chance that the action has already completed before the first state check, but the resolve/rejection only occurs `tb.await` returns. Resolves: #467 (comment)
ankur22
added a commit
that referenced
this pull request
Aug 9, 2022
We just want to make sure that the promises actually complete and are fulfilled before we end the test, so we're checking the state of the promise in `tb.await` as well as after `tb.await`. There's a chance that the action has already completed before the first state check, but the resolve/rejection only occurs `tb.await` returns. Resolves: #467 (comment)
ankur22
force-pushed
the
feat/440-async-waitfornavigation
branch
from
17f107f to
262eaee
Compare
ankur22
reviewed
Aug 9, 2022
ankur22
added a commit
that referenced
this pull request
Aug 11, 2022
We just want to make sure that the promises actually complete and are fulfilled before we end the test, so we're checking the state of the promise in `tb.await` as well as after `tb.await`. There's a chance that the action has already completed before the first state check, but the resolve/rejection only occurs `tb.await` returns. Resolves: #467 (comment)
ankur22
force-pushed
the
feat/440-async-waitfornavigation
branch
from
e63220b to
8b23121
Compare
We just want to make sure that the promises actually complete and are fulfilled before we end the test, so we're checking the state of the promise in `tb.await` as well as after `tb.await`. There's a chance that the action has already completed before the first state check, but the resolve/rejection only occurs `tb.await` returns. Resolves: #467 (comment)
This will ensure that the envetloop has complete before starting it up again. We're going to avoid placing lock here, since we want to follow the eventloop API and not allow multiple threads accessing the one eventloop.
ankur22
force-pushed
the
feat/440-async-waitfornavigation
branch
from
8b23121 to
61b84ee
Compare
ankur22
added a commit
that referenced
this pull request
Aug 11, 2022
We shouldn't need the CI specific timeout since the test should now be less flaky and is asynchronous. Resolves: #467 (comment)
We shouldn't need the CI specific timeout since the test should now be less flaky and is asynchronous. Resolves: #467 (comment)
ankur22
force-pushed
the
feat/440-async-waitfornavigation
branch
from
1e9fda0 to
fac54bd
Compare
imiric
reviewed
Aug 12, 2022
ankur22
force-pushed
the
feat/440-async-waitfornavigation
branch
from
f510cff to
3ea0c25
Compare
imiric
approved these changes
Aug 12, 2022
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Please feel free to add additional commits to the PR if you need to and merge it without me.
Note: I would create another PR to fix the problem in
k6ext.promise, but since I'll be leaving soon, I wanted to pack it in the same PR.Closes #440.