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

bug-1885442: Fail platform specific system tests when unexpectedly executed #985

Merged
merged 2 commits into from Mar 20, 2024

Conversation

relud
Copy link
Member

@relud relud commented Mar 14, 2024

No description provided.

@relud relud requested a review from a team as a code owner March 14, 2024 22:05
@relud relud requested a review from willkg March 14, 2024 22:07
@relud relud force-pushed the systemtest-cloud-provider-fail branch from a42fc3a to 43721af Compare March 14, 2024 22:08
@relud relud force-pushed the systemtest-cloud-provider-fail branch from 43721af to 08b062b Compare March 14, 2024 22:15
Copy link
Collaborator

@willkg willkg left a comment

Choose a reason for hiding this comment

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

r+wc. This works really nicely. Thank you!

# Set POSTURL to the url to post to.
#
# Set NONGINX=1 if you're running against a local dev environment. This
# will skip tests that require nginx.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you for fixing this!

@@ -0,0 +1,6 @@
[pytest]
addopts = -m 'not gcp'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Because we added this pytest.ini, it takes precedent over the configuration in the setup.cfg in the root so we need to add the pytest options here.

Suggested change
addopts = -m 'not gcp'
+addopts = -m 'not gcp' -rsxX --tb=native --showlocals
arg meaning
--showlocals show local variables in tracebacks
--tb=native Python standard library formatting
-r show summary info s skipped, x failed, X passed

I think I have pytest configured this way across all the services. I'm game for revisiting what args we want to use. The thing I really wanted here was the list of skipped tests in the test summary.

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, i didn't know setup.cfg was being picked up. should i just move the stuff i added to pytest.ini there?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The stuff you added only applies to the systemtests, so it needs to be in some pytest configuration file in the systemtests directory.

Relatedly, pytest can pick up configuration from a million places and I think I configured all our services inconsistently for whatever reasons which is confusing. Maybe we should standardize on putting pytest configuration only in pytest.ini files?

Copy link
Member Author

@relud relud Mar 18, 2024

Choose a reason for hiding this comment

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

The stuff you added only applies to the systemtests, so it needs to be in some pytest configuration file in the systemtests directory.

after some thought I disagree, I think this should go in setup.cfg (or whatever pytest config file applies to the whole repo in the future) for a few reasons:

  • I only added pytest.ini because i thought systemtest was the test root and that setup.cfg wasn't impacting systemtest. I would actually like to preserve the existing test root because it means that both systemtest and regular tests can be run with a single invocation of pytest by overriding the default testpaths, which I previously did not know and would very much like to retain so that I can easily run systemtests from vscode with the debugger enabled.
  • I only configured markers in pytest.ini, and markers always only apply to a subset of tests, even in this case it only applies to a specific file: systemtest/test_post_crash.py.
    • if the other tests ever did test something that depended on the environment at startup like in socorro, it could be appropriate to use this same marker there. I even worded the doc string to cover cases like this on purpose.
    • I could be convinced to go the other direction and put the config in code in either conftest.py or systemtest/test_post_crash.py, effectively restricting it from being picked up when systemtest isn't in testpaths.
  • I don't like duplicating configuration in a subdir, feels like asking for it to get out of date with our standards.

Maybe we should standardize on putting pytest configuration only in pytest.ini files?

i think we should standardize on pyproject.toml, it seems to be the current idiomatic place.

Copy link
Member Author

Choose a reason for hiding this comment

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

so it needs to be in some pytest configuration file in the systemtest directory.

also in case i misunderstood what you meant by needs above, in a technical sense it functions fine to put it in setup.cfg, that's not a problem here, I checked.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, I think I understand what's going on.

If we're going to have all the configuration in the root of the repo and the systemtests don't need any additional systemtest-specific configuration, then moving it all to pyproject.toml seems like a good idea. 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

moved pytest config to pyproject.toml, and filed https://bugzilla.mozilla.org/show_bug.cgi?id=1886536 to clean up whats left in setup.cfg and maybe setup.py

@relud relud merged commit b700047 into main Mar 20, 2024
1 check passed
@relud relud deleted the systemtest-cloud-provider-fail branch March 20, 2024 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants