Move conformance tests to a package extension

[lgoettgens]

Resolves #1936.

Changes:

• Add a package extension TestExt that depends on Test being loaded. For older julia versions, this uses Requires.jl.
• The conformance tests get moved to a module ConformanceTests with function stubs and helpers, the implementations are in the TestExt.
• test_elem is now a function stub in ConformanceTests, while the methods were moved to the respective src files
• test_elem got renamed generate_element (name open for discussion)
• The former files test/conformance-tests.jl, test/Groups-conformance-tests.jl, and test/Rings-conformance-tests.jl are only left for backwards compatibility. They load the relevant new files, and import things into the calling module (sometimes with a rename of things). They can be removed in an upcoming release, once the downstream packages have been adapted.

[codecov]

Codecov Report

Attention: Patch coverage is 95.12448% with 47 lines in your changes missing coverage. Please review.

Project coverage is 88.34%. Comparing base (66517ca) to head (b154ce4).
Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
ext/TestExt/Rings-conformance-tests.jl	94.74%	30 Missing ⚠️
ext/TestExt/Groups-conformance-tests.jl	91.37%	15 Missing ⚠️
src/generic/FunctionField.jl	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1954      +/-   ##
==========================================
+ Coverage   88.18%   88.34%   +0.15%     
==========================================
  Files         119      124       +5     
  Lines       30447    31400     +953     
==========================================
+ Hits        26849    27739     +890     
- Misses       3598     3661      +63     

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

[fingolfin]

Thanks!

Renaming test_elem is a good thing, finding a better name is tough. Personally I am not a fan of pseudo_random_element (disliking quasi-pseudo-hemi-demi-semi names in general, they are fuzzy and meaningless, no offense; also GAP already has a PseudoRandom with very different semantics).

I think we should move away from the "random" bit. The idea is less about producing "random" data and more about producing / generating data that is "useful for testing". This may in particular focus on ensuring certain "important" values are generated frequently enough, such as corner and edge cases (e.g. when generting an Int it is often a good idea to include 0 but also typemin(Int) and typemax(Int), etc.)

In specification resp. property based testing (think QuickCheck, Hypothesis, etc.) I think the term "generator" is used, although it it still a slightly different (and more powerful!) concept. (Honestly I've been wondering for some time if it might be a good idea to look into using something like https://github.com/Seelengrab/Supposition.jl to get test case shrinking... but that's a discussion for another day ;-).

Anyway, how about generate_element or just gen_elem or so? (IMHO the names don't need to quite follow our general naming rules as these are more internal. At the end of the day adding a good docstring explaining what a good method for this should do will be more helpful to developers.)

Then we could later potentially add generate_nilpotent_element resp. gen_nilp, or generate_unit / generate_invertible_element resp gen_unit etc.

[lgoettgens]

Anyway, how about generate_element or just gen_elem or so? (IMHO the names don't need to quite follow our general naming rules as these are more internal. At the end of the day adding a good docstring explaining what a good method for this should do will be more helpful to developers.)

Done

[thofma]

I wonder whether the methods for ConformanceTests.generate_element should be in src/? I would have expected them to be in the appropriate test file.

[lgoettgens]

I would have expected them to be in the appropriate test file.

The problem with that is that it makes composability very hard. Consider S::FreeAssociativeAlgebra{QQFieldElem} (with both AA and Nemo). Its generate_element method delegates to the one of base_ring(S).
For running the conformance tests of S, this would imply that one needs to first load a file from AA/test/ and Nemo/test/ such that both generate_element methods are available. And IMO the goal of this whole PR is to not need to include some random files from the test/ directory nowhere.

[fingolfin]

Didn't realize we had this function; so this remarks is not directly for this PR, but I also don't forget: I think we can delete it and replace all calls to it by calls to is_associated

[fingolfin]

Do we gain anything by adding the explicit ConformanceTest.?

How about we instead add export test_Ring_interface to module ConformanceTest and then to use conformance tests one does using .ConformanceTest (or something in that vein) instead of the current include(joinpath(pathof(AbstractAlgebra), "..", "..", "test", "Rings-conformance-tests.jl")) but otherwise doesn't have to change anything...

Well, except for renaming test_elem methods. But I am tempted to say that ConformanceTest should for now also do const test_elem = generate_element; export test_elem just to ease the transition. Thoughts?

[lgoettgens]

Do we gain anything by adding the explicit ConformanceTest.?

How about we instead add export test_Ring_interface to module ConformanceTest and then to use conformance tests one does using .ConformanceTest (or something in that vein) instead of the current include(joinpath(pathof(AbstractAlgebra), "..", "..", "test", "Rings-conformance-tests.jl")) but otherwise doesn't have to change anything...

I am happy to add some exports to the ConformanceTest module, but we have to draw a line somewhere. e.g. I really don't want to export things like equality or even generate_element (as the qualification at use-sites helps readers to remember what these functions are used for).

Well, except for renaming test_elem methods. But I am tempted to say that ConformanceTest should for now also do const test_elem = generate_element; export test_elem just to ease the transition. Thoughts?

I explicitly decided against, as we can use the change in including the conformance tests to also then rename test_elem. Otherwise, we get twice as much ugly glue code that needs to stay until some breaking release. Adapting downstream to the other needed changes (apart from the inclusion) should be pretty easy to do. I will prepare these, once this PR here is merged.

[thofma]

I am in favor of not exporting anything.

[fingolfin]

I agree that equality should not be exported (the name is really bad for that). The functions I think would be useful to export are the test_*_conformance functions. This way changes to adapt existing code to use the new package extension become minimal.

@thofma I don't understand what AbstractAlgebra.ConformanceTest should not export anything? After all one still has the option to do import AbstractAlgebra.ConformanceTest. To be clear, I am not suggesting that using AbstractAlgebra should export any of those functions (I don't even think it could, given that this is a package extensions, but in any case I don't want it).

[thofma]

I would have expected them to be in the appropriate test file.

The problem with that is that it makes composability very hard. Consider S::FreeAssociativeAlgebra{QQFieldElem} (with both AA and Nemo). Its generate_element method delegates to the one of base_ring(S). For running the conformance tests of S, this would imply that one needs to first load a file from AA/test/ and Nemo/test/ such that both generate_element methods are available. And IMO the goal of this whole PR is to not need to include some random files from the test/ directory nowhere.

Makes sense

[lgoettgens]

Can we please merge this and continue discussions about what should be exported afterward? Otherwise, this will just become stale and get conflicts.

[fingolfin]

Let's get it merged and tune it afterwards

[lgoettgens]

rebased due to conlficts