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

Turn ConstPolyRing example into a doctest #1894

Merged
merged 5 commits into from
Nov 11, 2024
Merged

Conversation

fingolfin
Copy link
Member

@fingolfin fingolfin commented Nov 7, 2024

... so that we verify it is correct there, instead of testing a copy which is out-of-sync.

This grew out of PR #1885 where I noticed that the two copies were already out of sync, and since I wanted to be able to test the show modifications for ConstPolyRing, which are therefore also included here (in corrected form compared to the untested code I had in #1885).

Copy link

codecov bot commented Nov 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.17%. Comparing base (f90e000) to head (aef2f5a).
Report is 6 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1894      +/-   ##
==========================================
+ Coverage   88.14%   88.17%   +0.03%     
==========================================
  Files         120      120              
  Lines       30288    30302      +14     
==========================================
+ Hits        26697    26719      +22     
+ Misses       3591     3583       -8     

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

... so that we verify it is correct *there*, instead of testing a copy which is out-of-sync

We can test it like this.

```jldoctest ConstPoly; filter = r".*"
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 this filter (and the one above in line 1018) is really meant to discard all output. This is because the output in test_EuclideanRing_interface comes from @testset. But there is only output if this is the top-most @testset. But it turns out that depending on how doctests are tested, they may either all be run in a single encompassing @testset, or not. Thus in one code path Documenter will complain here if I provide example output, and in the other if I don't provide it. The filter is the only one out.

Luckily this does not impeded the effectiveness of this test: If test_EuclideanRing_interface reports a failure, it still gets reported at least in the case with the encompassing @testset. I verified this by deliberately breaking one of the methods and checking that doctest reports this failure.

@lgoettgens
Copy link
Collaborator

I think your problem here is that doctestfilters are applied on a per-line basis by default. This means that if you have .* as the filter, the doctest still checks that the number of output lines is equal (since in both expected and evaluated output all output in each line is removed, but the new-line chars remain.
If you instead create a multine-matching regex (like r".*"m), this extra hack shouldn't be needed. just FYI, we already use a multinline-regex-doctestfilter in https://github.com/oscar-system/Oscar.jl/blob/194d82b312439e52c49b8b957ad68cb333bd39b7/src/utils/tests.jl#L154

@fingolfin
Copy link
Member Author

I just tried locally to revert the final hack and use r".*"m but it didn't work.

@fingolfin
Copy link
Member Author

Perhaps we can merge it with the gross hack and if one day someone figures out how to do without it, they can submit it as a follow-up?

@lgoettgens
Copy link
Collaborator

I looked into it and adapted the regex in such a way that it works locally without the hack. Let's see if CI agrees.

Reasoning: The m in r".*"m just treats the input as a multiline, instead of a set of single lines. However, . still does not match the \n char. To allow that, one must use the s modifier. See ?@r_str for more details.

@lgoettgens
Copy link
Collaborator

If CI is happy, I am fine with it

@fingolfin fingolfin merged commit d43940e into master Nov 11, 2024
29 checks passed
@fingolfin fingolfin deleted the mh/ConstPolyRing branch November 11, 2024 14:48
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