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

tests: Use options and fixtures to control all browsers #1300

Merged
merged 2 commits into from
Jun 21, 2024

Conversation

jsfehler
Copy link
Collaborator

@jsfehler jsfehler commented Jun 17, 2024

This PR refactors the tests to:

  • Remove the class based-structure and replace it with browser flags and isolated fixtures.
  • Replace an autouse fixture for visiting the dummy site with an explicit call in each test. This makes it easier to understand each test in isolation.
  • Run more tests for each browser.
  • Use a standard structure for xfail and skip instead of the macos tests subclassing tests.
  • Use a fixture to get the dummy app's URL instead of importing from it's internals.
  • Improve test organization by splitting them into more files and directories based on what they do.
  • Remove unnecessary imports from the requirements file for the tests

Before vs This PR:

  • Firefox: 446 vs 504
  • Chrome: 443 vs 498
  • Edge: 443 vs 498
  • Remote Chrome & Firefox: 380 + 2 skip vs 496
  • Remote Safari: 139 + 1 skip + 14 xfail + 41 error vs 229 + 3 skip + 13 xfail + 3 xpass
  • Django + flask + zope: 498 + 7 skip vs 499

@jsfehler jsfehler requested a review from fsouza June 17, 2024 18:42
Copy link
Contributor

@fsouza fsouza left a comment

Choose a reason for hiding this comment

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

woah this is large, but generally looks good. I'm approving after skimming through the code and given that CI is green 😁

@jsfehler jsfehler merged commit 68b96cd into cobrateam:master Jun 21, 2024
26 checks passed
@jsfehler
Copy link
Collaborator Author

woah this is large, but generally looks good. I'm approving after skimming through the code and given that CI is green 😁

Yeah, sorry about that. I did as much as I could over separate PRs but getting it done in one shot made addressing issues a lot easier without putting in temporary hacks.

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.

2 participants