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

Fix consistently failing test: Selector with a wait #135

Merged
merged 4 commits into from
Mar 9, 2024

Conversation

nielthiart
Copy link
Contributor

@nielthiart nielthiart commented Dec 18, 2023

On moderately slow networks, the "selector with a wait" test fails.

This commit replaces the --wait 2000 argument with --wait-for "!!document.querySelector('section.secondary')".

Edit: Reuses the --wait-for test's HTML with a delayed div.

Fixes #134


📚 Documentation preview 📚: https://shot-scraper--135.org.readthedocs.build/en/135/

@simonw
Copy link
Owner

simonw commented Feb 5, 2024

I think you forgot to include the examples/div-after-2-seconds.html file.

On moderately slow networks, the "selector with a wait" test fails.

This commit replaces the `--wait 2000` argument with `--wait-for "!!document.querySelector('section.secondary')"`.

Fixes simonw#134
On moderately slow networks, the "selector with a wait" test fails.

There is already a --wait-for test, so replacing the --wait test with --wait-for was a bad idea.

This change reuses the --wait-for test's HTML with a delayed div instead.

This way --wait with --selector is still tested.

Fixes simonw#134
@nielthiart nielthiart force-pushed the fix-test-wait-selector branch from 6e1bb3d to 641ba60 Compare February 6, 2024 08:51
Maybe having a unique name for each test will reduce ambiguity when discussing tests later on.

Ref simonw#134
@nielthiart
Copy link
Contributor Author

I think you forgot to include the examples/div-after-2-seconds.html file.

The examples/div-after-2-seconds.html is still created by a prior test step, but re-used in the new tests.

@nielthiart
Copy link
Contributor Author

Thanks for considering this PR. I realised now that I made a different mistake — I removed the original owlsnearme.com test completely, which was not my intention. This commit undoes that.

Hopefully, that will get squashed away on merge, but please let me know if I should squash everything to clean this small change up.

Taken as a whole, this update should now:

  1. Change the Selector with a wait for remote page element test to wait for a section to load, instead of a timed wait.
  2. Add two new tests that wait for a delayed div in examples/div-after-2-seconds.html.

This way we still test timed waiting, just not for remote pages.

@simonw
Copy link
Owner

simonw commented Mar 9, 2024

Thanks very much for this

@simonw simonw merged commit 0268b0c into simonw:main Mar 9, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failing test: Selector with a wait
2 participants