Update, add and refactor waitForURL and waitForNavigation tests
#5052
Conversation
5b13e80 to
38d26a8
Compare
| `) | ||
| require.NoError(t, err) | ||
|
|
||
| // Test when already at matching URL (should just wait for load state) |
There was a problem hiding this comment.
Should I make this into a table test?
There was a problem hiding this comment.
It might make sense to have it
e4a7e29 to
db6537e
Compare
| assert.False(t, checked) | ||
| } | ||
|
|
||
| //nolint:tparallel |
There was a problem hiding this comment.
This is unexpected. Do we really need? Can't we mitigate it? How can we guarantee that we are not hiding a race condition here?
If this is needed, then the comment should document The Why.
There was a problem hiding this comment.
So here's my motivation for doing it:
- Parallelising browser integration tests i think is quite expensive due to them needing their own chrome instance. So i personally think we should possibly reconsider this approach with browser tests, mainly in CI. I don't have numbers though, so i can't tell you whether it's a good assumption.
- I've started to prefer trying to perform a single setup function at the top, with only the important test logic in the test block in
for _, tt := range tests.
Since we've not discussed this before i'll change it to working with parallel tests for now.
Done in 58fcc25
There was a problem hiding this comment.
Yep, I have the feeling the problem is quite complex and we should open an issue identifying the problem. I see various possible ways for addressing it. For example, did we consider to have a pool of browsers to use? It would automatically rate limit the parallelism.
However, any assumption if not supported by data could be difficult to justify.
I don't have numbers though, so i can't tell you whether it's a good assumption.
So this sounds a requirement if we want to optimize in any direction.
However, if the decision is to stop the parallelism then I'd prefer if we add a condition for the entire package. So we don't have to add it for each test.
1cf6276 to
58fcc25
Compare
The implementation of waitForURL and url option on waitForNavigation no longer require the need to call page.close. The taskqueue is closed when the methods resolve/reject.
Each sub test is separated into it's own async run function so that we can easily debug issues with tests.
58fcc25 to
b8a0870
Compare
What?
This PR aims to tidy up the tests and add new ones where necessary including a refactor in the implementation for
waitForURL:parseStringOrRegexinwaitForURL;waitForURLtests toframe.Why?
Refactors based on comments made in this PR: #4920
Checklist
make check) and all pass.Checklist: Documentation (only for k6 maintainers and if relevant)
Please do not merge this PR until the following items are filled out.
Related PR(s)/Issue(s)
#4920