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

Remove tests on standard output #222

Merged
merged 1 commit into from
Dec 20, 2023
Merged

Conversation

SeanBryan51
Copy link
Collaborator

This change removes any assertions on standard output from the tests so that tests focus on testing functionality (where possible) rather than the messages (print statements) that are emitted from the tested functions.

Fixes #221

This change removes any assertions on standard output from the tests so
that tests focus on testing functionality (where possible) rather than
the messages (print statements) that are emitted from the tested
functions.

Fixes #221
@SeanBryan51 SeanBryan51 self-assigned this Dec 19, 2023
@SeanBryan51 SeanBryan51 linked an issue Dec 19, 2023 that may be closed by this pull request
Copy link

codecov bot commented Dec 19, 2023

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (9a8ec67) 82.68% compared to head (04f651f) 82.42%.
Report is 3 commits behind head on main.

Files Patch % Lines
benchcab/benchcab.py 0.00% 3 Missing ⚠️
benchcab/fluxsite.py 90.00% 1 Missing ⚠️
benchcab/model.py 83.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #222      +/-   ##
==========================================
- Coverage   82.68%   82.42%   -0.26%     
==========================================
  Files          28       28              
  Lines        1496     1417      -79     
==========================================
- Hits         1237     1168      -69     
+ Misses        259      249      -10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@bschroeter bschroeter left a comment

Choose a reason for hiding this comment

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

OK so the tests have been removed - thanks. But did we want to look at reimplementing them with subprocess-mock? Or update to test end-to-end (i.e. existence of files etc.) rather than on stdout?

@SeanBryan51
Copy link
Collaborator Author

I'm happy to switch to subprocess-mock in our unit testing however this might be better to do in a separate PR. Is this what you are suggesting?

@bschroeter
Copy link
Collaborator

I'm happy to switch to subprocess-mock in our unit testing however this might be better to do in a separate PR. Is this what you are suggesting?

That makes sense. Let's just accept that coverage will go down, but we should raise another issue to re-engineer these tests to ensure that we're covered for the functionality going forward.

@SeanBryan51
Copy link
Collaborator Author

Also just to clarify, is subprocess-mock referring to the pytest-subprocess pytest plugin that you have been working with?

@SeanBryan51 SeanBryan51 merged commit e6fed7d into main Dec 20, 2023
3 of 4 checks passed
@SeanBryan51 SeanBryan51 deleted the 221-update-the-tests-for-stdout branch December 20, 2023 01:23
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.

Update the tests for stdout.
2 participants