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

Add GAP distro tests #1067

Open
wants to merge 26 commits into
base: master
Choose a base branch
from
Open

Conversation

lgoettgens
Copy link
Member

@lgoettgens lgoettgens commented Nov 21, 2024

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
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

codecov bot commented Nov 21, 2024

Codecov Report

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

Project coverage is 74.16%. Comparing base (fda88b3) to head (e3e5907).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
src/packages.jl 0.00% 32 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1067      +/-   ##
==========================================
- Coverage   74.69%   74.16%   -0.54%     
==========================================
  Files          55       55              
  Lines        4636     4668      +32     
==========================================
- Hits         3463     3462       -1     
- Misses       1173     1206      +33     
Files with missing lines Coverage Δ
src/packages.jl 37.83% <0.00%> (-7.92%) ⬇️

... and 1 file with indirect coverage changes

@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
@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
Member

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

#- gap-package: 'packagemanager' # test failure
- gap-package: 'polenta' # `AL_EXECUTABLE, the executable for PARI/GP, has to be set`
- gap-package: 'polycyclic' # ???
- gap-package: 'polymaking' # `polymake command not found. Please set POLYMAKE_COMMAND by hand`
Copy link
Member

Choose a reason for hiding this comment

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

We could make polymake available, too, but it requires more work, see https://github.com/oscar-system/Oscar.jl/pull/4132/files

src/packages.jl Outdated Show resolved Hide resolved
@fingolfin
Copy link
Member

Good: gapdoc and wpe pass now.

Bad: most other packages now fail despite reporting success:

#I  No errors detected while testing

Test Failed at none:1
  Expression: GAP.Packages.test("automata")

ERROR: There was an error during testing

@lgoettgens
Copy link
Member Author

This is probably due to the return value handling. I'll look into it tomorrow

@lgoettgens
Copy link
Member Author

Ok, so the problem is how gap packages try to exit after running their tests.

Most packages call TestDirectory with exitGAP:=true (e.g. https://github.com/gap-packages/datastructures/blob/24f8ea8d4265ffb64a6216a57fabf3aee739cc15/tst/testall.g#L4) (which internally calls QuitGap with an exit code. Up to 5587398 (#1067) we used this exit code to determine if the tests succeeded. Afterwards, these packages call ForceQuitGap(1) as they think this is an unreachable statement and thus an error if reached (cf https://github.com/gap-packages/datastructures/blob/24f8ea8d4265ffb64a6216a57fabf3aee739cc15/tst/testall.g#L7). Due to 5587398 (#1067), we now fail due to this exit code of 1.

wpe does not use TestDirectory, but instead calls ForceQuitGap with an exit code indicating if the tests succeeded (cf. https://github.com/gap-packages/WPE/blob/adc54d808f8b1634c227719f2b41a6fe81373c41/tst/testall.g#L27-L31).

Ways to solve this:

  1. Patch wpe to use QuitGap instead of ForceQuitGap.
  2. Only consider the very first call to any of QuitGap and ForceQuitGap as the try to convey an exit code and ignore all further calls (as they are unreachable in usual GAP execution anyway).

I will try to implement 2.

@fingolfin
Copy link
Member

Option 2 to me sounds like the way to go as it models more accurately what GAP does.

using Artifacts, TOML;
output = sprint(print, "gap-packages=", map(name -> name[9:end], sort!(collect(filter(startswith("GAP_pkg_"), keys(TOML.parsefile(find_artifacts_toml(Base.active_project()))))))));
println(output);
open(ENV["GITHUB_OUTPUT"], "a") do io;
Copy link
Member

Choose a reason for hiding this comment

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

Not important but FYI I don't think you need the semicolons here

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I added these to easier copy paste this into my console for debugging ans not getting spammed with output

@lgoettgens
Copy link
Member Author

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

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