Skip to content

Ensure more tests pass with gap --bare#6387

Merged
fingolfin merged 6 commits into
masterfrom
mh/prep-tests-for-bare
May 11, 2026
Merged

Ensure more tests pass with gap --bare#6387
fingolfin merged 6 commits into
masterfrom
mh/prep-tests-for-bare

Conversation

@fingolfin
Copy link
Copy Markdown
Member

  • remove a bunch of SmallGroup and IdGroup uses
  • remove tst/testbugfix/2005-05-03-t00070.tst (an equivalent test was added to the smallgrp package recently)
  • modify some .tst file to run parts only if certain packages are available, via #@if IsPackageMarkedForLoading(...)
  • don't load ctbllib (and all its dependencies) in ctblmaps.tst

Extracted from PR #6346

- remove a bunch of SmallGroup and IdGroup uses
- remove tst/testbugfix/2005-05-03-t00070.tst (an equivalent
  test was added to the smallgrp package recently)
- modify some .tst file to run parts only if certain packages
  are available, via `#@if IsPackageMarkedForLoading(...)`
- don't load `ctbllib` (and all its dependencies) in `ctblmaps.tst`
@fingolfin fingolfin requested a review from ThomasBreuer May 9, 2026 19:52
@fingolfin fingolfin added topic: tests issues or PRs related to tests release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels May 9, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented May 9, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.68%. Comparing base (4e3501f) to head (aa9176f).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6387      +/-   ##
==========================================
- Coverage   78.69%   78.68%   -0.02%     
==========================================
  Files         684      683       -1     
  Lines      292902   292902              
  Branches     8665     8658       -7     
==========================================
- Hits       230508   230462      -46     
- Misses      60575    60621      +46     
  Partials     1819     1819              

☔ 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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Copy Markdown
Contributor

@ThomasBreuer ThomasBreuer left a comment

Choose a reason for hiding this comment

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

Looks good, just a few comments.

So the message is:

In test files for the GAP library,

  • do not call LoadPackage,
  • if it is necessary to call functions from a package in a test then surround this test with comment lines #@if IsPackageMarkedForLoading( ... ) and #@fi mentioning the package in question.

Comment thread tst/testinstall/opers/SimpleGroup.tst Outdated
Comment thread tst/testinstall/opers/SimpleGroup.tst Outdated
Comment thread tst/testinstall/opers/SimpleGroup.tst Outdated
Comment thread tst/testinstall/opers/SimpleGroup.tst Outdated
Comment thread tst/testinstall/opers/SimpleGroup.tst Outdated
Comment thread tst/testinstall/opers/SimpleGroup.tst Outdated
# MTX.HomogeneousComponents
#
gap> G:=SmallGroup(24, 3);;
gap> G:=SL(2, 3);;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Here a pc group gets replaced by a matrix group.
I think this is not a problem for the test.

Comment thread tst/testinstall/ctbl.tst Outdated
Comment thread tst/testinstall/opers/MinimalNormalSubgroups.tst Outdated
gap> SortedList(List(SubdirectProducts(G, G), StructureDescription));
[ "(C3 x C3) : C2", "S3", "S3 x S3" ]
gap> SortedList(List(SubdirectProducts(H, H), Size));
[ 24, 96, 288, 576 ]
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This one is a bit unfortunate, but StructureDescription isn't quite stable on some of these groups. Anyway, I don't think we loose much by this change; it's unlikely a bug would keep all the other structures right, and the sizes here, but mess up the isomorphism type of one of the outputs in this one example somehow. Not impossible of course; but then one could say the same even with this test was as it was before.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Perhaps a comment in the test file that explains the situation would be useful.
(In a few experiments, I did not see an instability in the StructureDescription output.)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'll add a comment. It seems to depend on which packages are present: with my "default" GAP, I see

gap> H := SymmetricGroup(4);;
gap> SortedList(List(SubdirectProducts(H, H), StructureDescription));
[ "((C2 x C2 x C2 x C2) : C3) : C2", "(A4 x A4) : C2", "S4", "S4 x S4" ]

When I use gap -A, I get

gap> H := SymmetricGroup(4);;
gap>SortedList(List(SubdirectProducts(H, H), StructureDescription));
[ "(C2 x C2) : ((C3 x A4) : C2)", "(C2 x C2) : S4", "S4", "S4 x S4" ]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah, this kind of instability is meant, dependence on loaded packages.
Thanks for the comments.

@fingolfin fingolfin merged commit f9d2d84 into master May 11, 2026
32 of 33 checks passed
@fingolfin fingolfin deleted the mh/prep-tests-for-bare branch May 11, 2026 10:13
cdwensley pushed a commit that referenced this pull request May 12, 2026
- remove a bunch of SmallGroup and IdGroup uses in tst files
- remove tst/testbugfix/2005-05-03-t00070.tst (an equivalent
  test was added to the smallgrp package recently)
- modify some .tst file to run parts only if certain packages
  are available, via `#@if IsPackageMarkedForLoading(...)`
- don't load `ctbllib` (and all its dependencies) in `ctblmaps.tst`
cdwensley pushed a commit that referenced this pull request May 12, 2026
- remove a bunch of SmallGroup and IdGroup uses in tst files
- remove tst/testbugfix/2005-05-03-t00070.tst (an equivalent
  test was added to the smallgrp package recently)
- modify some .tst file to run parts only if certain packages
  are available, via `#@if IsPackageMarkedForLoading(...)`
- don't load `ctbllib` (and all its dependencies) in `ctblmaps.tst`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes topic: tests issues or PRs related to tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants