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

fix test suite: de-doctest code block #35

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nsajko
Copy link
Contributor

@nsajko nsajko commented Sep 26, 2024

The code block was failing the test suite as a doc test. This was happening, specifically, because of the method overwrite warnings.

None of that code block is appropriate for being a doctest, e.g., it tests the exact form of the human-readable show output. So the fix is easy.

The code block was failing the test suite as a doc test. This was
happening, specifically, because of the method overwrite warnings.

None of that code block is appropriate for being a doctest, e.g., it
tests the exact form of the human-readable `show` output. So the fix is
easy.
Copy link

codecov bot commented Sep 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.98%. Comparing base (ed3e40c) to head (b6fc746).

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #35      +/-   ##
==========================================
- Coverage   98.14%   92.98%   -5.17%     
==========================================
  Files           2        3       +1     
  Lines          54       57       +3     
==========================================
  Hits           53       53              
- Misses          1        4       +3     

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

@coveralls
Copy link

Pull Request Test Coverage Report for Build 11061494112

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall first build on fix_test_suite at 94.643%

Totals Coverage Status
Change from base Build 7799682274: 94.6%
Covered Lines: 53
Relevant Lines: 56

💛 - Coveralls

@devmotion
Copy link
Member

devmotion commented Sep 27, 2024

None of that code block is appropriate for being a doctest

That's not true? IIRC it's supposed to demonstrate and test different ways of applying the @irrational macro for defining new AbstractIrrational constants, and to show and test that errors are thrown if it is applied incorrectly.

So I think it would be better to fix the warnings.

@nsajko
Copy link
Contributor Author

nsajko commented Sep 27, 2024

show and test that errors are thrown if it is applied incorrectly

There's @test_throws for that. No need to test the exact strings of output meant for humans, not meant to be machine readable.

@devmotion
Copy link
Member

Sure, one can use @test_throws. But the docstring was supposed to demonstrate workflows and errors you would get in the REPL, and Documenter explicitly supports such doctests.

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