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

improve pytest configuration #1700

Merged
merged 3 commits into from
Apr 24, 2023
Merged

improve pytest configuration #1700

merged 3 commits into from
Apr 24, 2023

Conversation

pmeier
Copy link
Member

@pmeier pmeier commented Apr 7, 2023

pytest's default configuration is horrible with respect to a few aspects:

  • traceback (--tb): default is "long" which not only shows a few lines above and below the one the error happened for each frame, but also the docstring for every stack. This easily buries the information I need in a lot of noise. "short" is better, but still pretty noisy. "native" is what Python gives you by default, which already includes a lot of information.
  • xpass (-rX / xfail_strict = True): By default, the @pytest.mark.xfail is not strict. This means whether the test fails or not, pytest will never return a non-zero exit code. Since most people just look for a boolean signal from CI this can easily hide errors. With the xfail_strict = True setting, unexpectedly passing tests cause the run to fail.
  • warnings (-Wd): pytest follows Pythons convention and adopts the same warning filters. This means for example that DeprecationWarning's are not shown. However that is very useful information for developers. Setting -Wd disables the filter and thus all warnings are shown.

This PR adapts the pytest.ini configuration file to have more sensible "defaults".

cc @kcpevey since we touched on this offline before.

Comment on lines +11 to +12
testpaths =
tests
Copy link
Member Author

Choose a reason for hiding this comment

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

This is more of a QoL improvement. with this one can type pytest without specifying anything else to run these tests. LMK if you want me to revert this.

Comment on lines +7 to +8
# enable all warnings
-Wd
Copy link
Member Author

@pmeier pmeier Apr 7, 2023

Choose a reason for hiding this comment

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

I personally would go a step further

Suggested change
# enable all warnings
-Wd
# turn warnings into errors
-We

Right now there aren't any warnings so this is a good time to adopt such a change. With this option set, we get an early heads-up if something is about the change in the future.

@pmeier
Copy link
Member Author

pmeier commented Apr 7, 2023

I also added the pytest.ini file the to CI workflow triggers

@pavithraes pavithraes added area: testing ✅ Testing needs: review 👀 This PR is complete and ready for reviewing type: maintenance 🛠 Day-to-day maintenance tasks labels Apr 12, 2023
@pmeier pmeier requested a review from kcpevey April 13, 2023 15:05
Copy link
Contributor

@kcpevey kcpevey left a comment

Choose a reason for hiding this comment

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

These look good to me. Not sure if we want one more approval on this. Maybe from @iameskild or @viniciusdc ?

@kcpevey kcpevey merged commit 217841f into nebari-dev:develop Apr 24, 2023
@kcpevey
Copy link
Contributor

kcpevey commented Apr 24, 2023

This has been open for a while. I approved it and didn't hear any negative feedback so I went ahead and merged.

@pmeier pmeier deleted the pytest-ini branch April 24, 2023 15:31
@pmeier pmeier mentioned this pull request May 2, 2023
10 tasks
@costrouc
Copy link
Member

costrouc commented May 2, 2023

Team agrees this is a useful commit to have warnings as errors.

@pmeier pmeier mentioned this pull request May 2, 2023
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: testing ✅ Testing needs: review 👀 This PR is complete and ready for reviewing type: maintenance 🛠 Day-to-day maintenance tasks
Projects
Development

Successfully merging this pull request may close these issues.

4 participants