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

Improve reliability of Click to Load integration tests #1182

Merged

Conversation

kzar
Copy link
Collaborator

@kzar kzar commented May 18, 2022

While they run reliably locally, some of the Click to Load integration
tests are flaking on CI. These improvements aim to help reduce those
flakes:

  • If a test page fails to load, let's mark the test as skipped rather
    than as a failure. There's not much we can do to get around network
    failure.
  • Take care to wait between unblocking a YouTube video and clicking
    to play it.
  • Take care to wait for pages to finish closing.
  • Avoid exceeding Jasmine's 20 second timeout when waiting for the
    extension configuration to load. If extension configuration fails
    to load, throw a clear exception to explain that.
  • When clicking the Facebook Click to Load buttons, ignore buttons
    that Puppeteer fails to click.
  • Fix a bug where the test extension configuration would still
    attempt to be unloaded, even when the test configuration failed to
    load.
  • When API schema tests timeout, mark them as skipped with a clear
    message.
  • If embedded Facebook content does not load within 15 seconds, time
    out but continue with the tests.
  • If spherical video roll test times out, mark the test as pending
    and log an appropriate message. This happens when the network is
    too slow and the video fails to load within 30 seconds.

Reviewer: @jonathanKingston

Steps to test this PR:

  1. Ensure integration tests pass more reliably.

Automated tests:

  • Unit tests
  • Integration tests
Reviewer Checklist:
  • Ensure the PR solves the problem
  • Review every line of code
  • Ensure the PR does no harm by testing the changes thoroughly
  • Get help if you're uncomfortable with any of the above!
  • Determine if there are any quick wins that improve the implementation
PR Author Checklist:
  • Get advice or leverage existing code
  • Agree on technical approach with reviewer (if the changes are nuanced)
  • Ensure that there is a testing strategy (and documented non-automated tests)
  • Ensure there is a documented monitoring strategy (if necessary)
  • Consider systems implications

@kzar kzar force-pushed the improve-ctp-integration-tests branch 3 times, most recently from 897f8d1 to a19e573 Compare May 18, 2022 15:16
While they run reliably locally, some of the Click to Load integration
tests are flaking on CI. These improvements aim to help reduce those
flakes:
 - If a test page fails to load, let's mark the test as skipped rather
   than as a failure. There's not much we can do to get around network
   failure.
 - Take care to wait between unblocking a YouTube video and clicking
   to play it.
 - Take care to wait for pages to finish closing.
 - Avoid exceeding Jasmine's 20 second timeout when waiting for the
   extension configuration to load. If extension configuration fails
   to load, throw a clear exception to explain that.
 - When clicking the Facebook Click to Load buttons, ignore buttons
   that Puppeteer fails to click.
 - Fix a bug where the test extension configuration would still
   attempt to be unloaded, even when the test configuration failed to
   load.
 - When API schema tests timeout, mark them as skipped with a clear
   message.
 - If embedded Facebook content does not load within 15 seconds, time
   out but continue with the tests.
 - If spherical video roll test times out, mark the test as pending
   and log an appropriate message. This happens when the network is
   too slow and the video fails to load within 30 seconds.
@kzar kzar force-pushed the improve-ctp-integration-tests branch from a19e573 to b579959 Compare May 18, 2022 16:25
Copy link
Collaborator

@jonathanKingston jonathanKingston left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the detailed comment, I'm happy that what you described is what it's doing and the test is now passing 🥳

Thanks for fixing this up so quickly!

@jonathanKingston jonathanKingston merged commit 77e7062 into duckduckgo:develop May 18, 2022
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.

None yet

2 participants