-
Notifications
You must be signed in to change notification settings - Fork 42
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
Refactor lifecycle tests and add any missing tests #647
Conversation
75b9107
to
c8b56ae
Compare
9106a4c
to
99980ef
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work 👏 Thanks for considering my previous feedback and refactoring this 🙇
I'm approving this PR as it is, and I have some suggestions I want you to consider. All are optional. I believe there are some areas in which we can further decrease duplication and make it more straightforward and maintainable.
Commits:
IMHO, we don't need to make separate commits for minor changes, such as the following. Please consider squashing them
- Add general lifecycle event guidelines
- Rename the goto network lifecycle event tests
- Move general guideline comment to top
- Refactor ... test commits
It is now clear that the same code exercises the test cases of a table test. And each test case is small enough. A commit for each top-level test is enough. IMO, no need to make a commit for each table test. It is better to squash them into one commit.
The following static/*.html
files are nearly identical.
- prolonged_network_idle.html
- prolonged_network_idle_10.html
- reload_lifecycle.html
- wait_for_nav_lifecycle.html
We can parameterize them for each test case instead of duplicating these files. You can use page.Eval
to do that. I found that only the following parts change:
Changing parts:
+ <div id="pingJSText">Waiting...</div>
+ Loop count
+ <script src="/ping.js" async></script>
+ <a href="/home" id="homeLink">Home</a>
Summary :)
As the PR is big, I couldn't validate its functionality. I'll check it out once again when I'm back.
99980ef
to
acc5906
Compare
Thanks for the suggestions! I'll take a look at them 🙂 |
acc5906
to
4f88417
Compare
840ba23
to
85bac97
Compare
85bac97
to
74a5871
Compare
We're deduplicating the ping handler to make it simpler to read the tests.
Deduplicate the ping.js handler in the lifecycle tests.
74a5871
to
74d5402
Compare
Deduplicate the home handler in the lifecycle tests.
Deduplicate them and convert them to a table test.
Deduplicate them and convert them to a table test.
These tests ensure that we receive the subframe events before the lifecycle events for the main/root frame. This will ensure that the waitUntil only unblocks once all frames are ready.
This is in preparation for the WaitForNavigation lifecycle tests.
This test locks in the behaviour which highlights that it will timeout if the function call is made after the navigation has ended.
Co-authored-by: İnanç Gümüş <[email protected]>
74d5402
to
6791e5b
Compare
Consolidates as much of the test functionality into one html file.
0dd3708
to
215000d
Compare
Before we attempt to cleanup the lifecycle event code (#593) we need to lock in the current behaviour so that any refactoring does not affect the behaviour.
The tests are:
waitUntil
onload
andDOMContentLoaded
work as expected when used ingoto
.waitUntil
onload
,DOMContentLoaded
andnetworkidle
work as expected when we navigate to a page with a main and sub frame when used ingoto
.waitUntil
onload
,DOMContentLoaded
andnetworkidle
work as expected when used as an option onWaitForNavigation
.waitUntil
onnetworkidle
timesout if we set it too late after the navigation has completed.Linked issue: #593