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

chore: Remove requirement for pytest #273

Merged
merged 2 commits into from
Oct 5, 2023

Conversation

giovanni-guidini
Copy link
Contributor

pytest was required only by the PythonStandardRunner, and only in the strict_mode.
No one uses that.
At first it was develop because we were concerned that users would be picky by us running pytest with subprocess.run,
so strict_mode was an alternative. Since then people are starting to use ATS and that hasn't been an issue.

Plus with custom runners we can solve this problem without the complications of requiring pytest.
And there's a possibility (although I think it's very unlikely) that importing pytest from the CLI
might cause some of the issues we were seeing when running tests from it in ATS. Who knows.

In any case, removing pytest and all code associated with it.

pytest was required only by the `PythonStandardRunner`, and only in the `strict_mode`.
No one uses that.
At first it was develop because we were concerned that users would be picky by us running pytest with `subprocess.run`,
so `strict_mode` was an alternative. Since then people are starting to use ATS and that hasn't been an issue.

Plus with custom runners we can solve this problem without the complications of requiring pytest.
And there's a possibility (although I think it's very unlikely) that importing pytest from the CLI
might cause some of the issues we were seeing when running tests from it in ATS. Who knows.

In any case, removing pytest and all code associated with it.
@giovanni-guidini giovanni-guidini force-pushed the gio/remove-pytest-requirement branch from c0b48b7 to d8b0850 Compare October 2, 2023 18:23
@codecov
Copy link

codecov bot commented Oct 2, 2023

Codecov Report

Merging #273 (9006dcd) into master (127e8af) will decrease coverage by 0.05%.
Report is 1 commits behind head on master.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #273      +/-   ##
==========================================
- Coverage   94.91%   94.86%   -0.05%     
==========================================
  Files          78       78              
  Lines        2694     2631      -63     
==========================================
- Hits         2557     2496      -61     
+ Misses        137      135       -2     
Flag Coverage Δ
python3.10 95.19% <100.00%> (-0.04%) ⬇️
python3.11 95.18% <100.00%> (-0.04%) ⬇️
python3.8 95.19% <100.00%> (-0.04%) ⬇️
python3.9 95.19% <100.00%> (-0.04%) ⬇️
smart-labels 94.86% <100.00%> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
codecov_cli/runners/python_standard_runner.py 100.00% <100.00%> (+1.39%) ⬆️
setup.py 0.00% <ø> (ø)

Copy link
Contributor

@joseph-sentry joseph-sentry left a comment

Choose a reason for hiding this comment

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

can _wait_pytest and the imports for Process and Queue be removed form python_standard_runner be removed as well? Otherwise, lgtm!

joseph-sentry
joseph-sentry previously approved these changes Oct 4, 2023
Copy link
Contributor

@joseph-sentry joseph-sentry left a comment

Choose a reason for hiding this comment

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

other than my previous comment, lgtm!

@giovanni-guidini giovanni-guidini merged commit 31278f2 into master Oct 5, 2023
10 checks passed
@giovanni-guidini giovanni-guidini deleted the gio/remove-pytest-requirement branch October 5, 2023 13:28
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