Skip to content

test: get rid of ring_to_mat - #1902

Open
fingolfin wants to merge 1 commit into
masterfrom
mh/ring_to_mat
Open

test: get rid of ring_to_mat#1902
fingolfin wants to merge 1 commit into
masterfrom
mh/ring_to_mat

Conversation

@fingolfin

Copy link
Copy Markdown
Member

The 'similar' methods being tested are generic and defined &
tested in AA.

Getting rid of this makes it somewhat less annoying to run the
affect test files interactively.

@codecov

codecov Bot commented Oct 17, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.10%. Comparing base (542a6be) to head (d0eaf8e).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1902      +/-   ##
==========================================
- Coverage   88.14%   88.10%   -0.05%     
==========================================
  Files         100      100              
  Lines       36852    36852              
==========================================
- Hits        32482    32467      -15     
- Misses       4370     4385      +15     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@lgoettgens

Copy link
Copy Markdown
Member

I agree that currently these tests are more or less useless. But these would make it very easy to test #1826 (once that is fixed) by just adding @test t isa M twice in each of the for-loops.

@fingolfin

Copy link
Copy Markdown
Member Author

Sure, but I think this should be handled by adding such a test to the ring conformance tests in AA.

@fingolfin fingolfin closed this Oct 30, 2024
@fingolfin fingolfin reopened this Oct 30, 2024
@fingolfin

Copy link
Copy Markdown
Member Author

To move forward for this, my solution would be to integrate a similar test into the conformance test suite. Specifically into test_MatSpace_interface.

To handle conversion to different rings, we could use something similar to the adhoc_partner_rings to provide a list of "partner" rings against which to perform the similar tests. Note that M should be redundant and just the same as dense_matrix_type(R).

@fingolfin

Copy link
Copy Markdown
Member Author

I believe this could be merged now...

@fingolfin
fingolfin requested a review from lgoettgens May 7, 2025 22:45
@lgoettgens

Copy link
Copy Markdown
Member

To move forward for this, my solution would be to integrate a similar test into the conformance test suite. Specifically into test_MatSpace_interface.

What about first adding that there and then removing it here afterwards? Otherwise, we just reduce code coverage for little benefit

@fingolfin

Copy link
Copy Markdown
Member Author

What about first adding that there and then removing it here afterwards? Otherwise, we just reduce code coverage for little benefit

That was already done last November, see Nemocas/AbstractAlgebra.jl#1890

@fingolfin

Copy link
Copy Markdown
Member Author

Oh no, wait, those are not for conversion to other rings. sigh

The 'similar' methods being tested are generic and defined &
tested in AA.

Getting rid of this makes it somewhat less annoying to run the
affect test files interactively.
@fingolfin

Copy link
Copy Markdown
Member Author

Alas, I am not sure how to generically implement this then. It seems we'd need a global list of "example ring objects" for this ?

@lgoettgens

Copy link
Copy Markdown
Member

Alas, I am not sure how to generically implement this then. It seems we'd need a global list of "example ring objects" for this ?

It seems so. And this list ist what this PR is removing 😅
Maybe one could do something similar as for the adhoc_partner_rings, see https://github.com/Nemocas/AbstractAlgebra.jl/blob/7fcf80bf8aa1f39ae11dfc5b921a48c37dd2d252/src/ConformanceTests.jl#L33

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