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

refactor(integration): replace pytest.skip with pytest markers #798

Merged
merged 1 commit into from
Nov 13, 2024

Conversation

Trinaa
Copy link
Contributor

@Trinaa Trinaa commented Nov 12, 2024

Proposing to use pytest markers instead of pytest.skip + environment variables for filtering integration tests. This will:

  • Allow for more accurate skip rate reporting in test metrics, since some tests are only intended for local execution
  • Will reduce cyclomatic complexity in the test cases
  • Will likely scale better if more filters are needed in future

As a workaround, I've added continued support for the environment variables in the Makefile.

Note: Tests that are filtered out are noted as 'deselected' in cmd output

image image

Closes: #SYNC-4490

An alternative could be introducing a marker like @pytest.mark.no_ci. We could then leave the pytest.skip + environment variables in place and still report skip rates accurately, but I find this less ideal because we would have multiple mechanisms for filtering tests. This complexity seems unnecessary.

Add-on: I removed the no-root option in the Makefile because we use package-mode = false and should no longer need it according to this warning.
image

@Trinaa Trinaa requested review from jrconlin and taddes November 12, 2024 15:52
jrconlin
jrconlin previously approved these changes Nov 12, 2024
taddes
taddes previously approved these changes Nov 12, 2024
Copy link
Contributor

@taddes taddes 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 doing this @Trinaa ! Not to be too picky, but to pick up from @jrconlin 's comment, I thought it might not hurt to also add a line in the test docs https://github.com/mozilla-services/autopush-rs/blob/master/docs/src/testing.md for clarity. I'll leave it up to you, but I kind of like us being in the habit of adding to the core service docs as well when we have a procedural or technical norm we want to establish.

@Trinaa Trinaa dismissed stale reviews from taddes and jrconlin via 0898f78 November 12, 2024 21:16
@Trinaa Trinaa force-pushed the replace_test_skip_with_tag branch from ce306dd to 0898f78 Compare November 12, 2024 21:16
@Trinaa Trinaa force-pushed the replace_test_skip_with_tag branch from 0898f78 to d265166 Compare November 12, 2024 21:49
Copy link
Contributor

@taddes taddes left a comment

Choose a reason for hiding this comment

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

🚀

@Trinaa Trinaa merged commit 84f1fe8 into master Nov 13, 2024
1 check passed
@Trinaa Trinaa deleted the replace_test_skip_with_tag branch November 13, 2024 14:20
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.

3 participants