Skip to content

Add GAP distro tests - #1067

Merged
fingolfin merged 31 commits into
oscar-system:masterfrom
lgoettgens:lg/CI-distro
Jan 7, 2025
Merged

Add GAP distro tests#1067
fingolfin merged 31 commits into
oscar-system:masterfrom
lgoettgens:lg/CI-distro

Conversation

@lgoettgens

@lgoettgens lgoettgens commented Nov 21, 2024

Copy link
Copy Markdown
Member

Resolves #1065.

This is dependent on #1066 and thus includes it.

I've used the ugly hack from oscar-system/GAP_pkg#15 in GAP.Packages.test to avoid GAP packages exiting the process after running their tests. This function could be moved to a package extension on Test once the minimum required julia version is bumped to 1.8.

@lgoettgens
lgoettgens force-pushed the lg/CI-distro branch 11 times, most recently from 7088374 to 12ce2c4 Compare November 21, 2024 19:22
name: Test GAP package distro

on:
pull_request:

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.

Suggested change
pull_request:

IMO this shouldn't run on every commit in every PR, as it produces >150 jobs. But I have kept it here for now such that in this PR, we can already see it running.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There are >150 jobs but apparently they take less than 15 minutes to run? So perhaps we keep them on for now and see what happens?

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.

Fine with me

@codecov

codecov Bot commented Nov 21, 2024

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 0% with 28 lines in your changes missing coverage. Please review.

Project coverage is 75.28%. Comparing base (99ddda8) to head (29c4f28).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
src/packages.jl 0.00% 28 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1067      +/-   ##
==========================================
- Coverage   75.73%   75.28%   -0.46%     
==========================================
  Files          55       55              
  Lines        4641     4669      +28     
==========================================
  Hits         3515     3515              
- Misses       1126     1154      +28     
Files with missing lines Coverage Δ
src/packages.jl 38.67% <0.00%> (-7.08%) ⬇️

@lgoettgens
lgoettgens marked this pull request as ready for review November 21, 2024 19:34
@lgoettgens
lgoettgens marked this pull request as draft November 22, 2024 09:14
Comment thread .github/workflows/CI-distro.yml Outdated
@lgoettgens
lgoettgens force-pushed the lg/CI-distro branch 7 times, most recently from 19c0f2b to c5a9136 Compare November 22, 2024 13:44
@lgoettgens

This comment was marked as outdated.

@lgoettgens
lgoettgens marked this pull request as ready for review November 22, 2024 13:55
@fingolfin

Copy link
Copy Markdown
Member

Thank you for this @lgoettgens. Could you please rebase it? I'll try to have a closer in the coming week(s)

Comment thread .github/workflows/CI-distro.yml Outdated
Comment thread .github/workflows/CI-distro.yml Outdated
Co-authored-by: Max Horn <max@quendi.de>
Comment thread .github/workflows/CI-distro.yml Outdated
Comment thread .github/workflows/CI-distro.yml Outdated
@lgoettgens

Copy link
Copy Markdown
Member Author

The help package still has some failures. Could you have a look at it?

@fingolfin

Copy link
Copy Markdown
Member

Hmm, there also was a segfault in the atlasrep test suite (looks GC related :-/)

And another in guava which looks like maybe an encoding issue (?) but which I also can't reproduce locally

########> Diff in GAPROOT/pkg/GAP_pkg_guava/guava3.19/tst/guava.tst:649
# Input is:
p := CodeIsomorphism( C1, C2 );
# Expected output:
(2,13,7,10,8,3,5,4,14)(12,15)
# But found:
Syntax error: expression expected in /tmp/gaptempfile.5zTo2m:1
GUAVA_TEMP_VAR := � &
                  ^
Syntax error: ; expected in /tmp/gaptempfile.5zTo2m:3
false
########

In order to move things along, how about we disable those for now and merge this, then file issues about these failures and then can investigate these later.

@lgoettgens

Copy link
Copy Markdown
Member Author

After re-running it is only help failing. I'll go ahead disabling this and anything that randomly fails in the next few CI runs.

@lgoettgens

Copy link
Copy Markdown
Member Author

All distro tests are green now. The one failure is #1111.

@lgoettgens
lgoettgens requested a review from fingolfin January 6, 2025 16:04
Comment thread src/packages.jl Outdated
Comment thread src/packages.jl Outdated
Comment thread src/packages.jl Outdated
Comment thread src/packages.jl Outdated
Comment thread src/packages.jl Outdated
Comment thread src/packages.jl Outdated
Comment thread src/packages.jl Outdated
Comment thread src/packages.jl Outdated
- gap-package: 'help' # test failure in HeLP-4.0/tst/yes_4ti2.tst:39
- gap-package: 'io' # segfaults, most likely due to child process handling
- gap-package: 'itc' # dependency `xgap` has no jll
- gap-package: 'localizeringforhomalg' # `Error, found no GAP executable in PATH`

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

See also issue #1105 (@ThomasBreuer that's another case where a package needs/wants a GAP executable)

@fingolfin
fingolfin enabled auto-merge (squash) January 7, 2025 08:14
@fingolfin
fingolfin merged commit cb8d2af into oscar-system:master Jan 7, 2025
@lgoettgens
lgoettgens deleted the lg/CI-distro branch January 7, 2025 09:14
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.

Systematic testing of all packages in the GAP distro

4 participants