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

CI: add a test kernel extension to mockpkg #4776

Merged
merged 1 commit into from
Feb 19, 2022
Merged

Conversation

fingolfin
Copy link
Member

This is preparation for PR #4746 but also useful on its own.

@fingolfin fingolfin added release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes topic: ci Anything related to GitHub Actions, Codecov, AppVeyor, Coveralls, Travis, ... labels Feb 18, 2022
@wilfwilson
Copy link
Member

The CI failure is unrelated and something that I have noticed a few times:

+ echo 'Running test suite testexpect'
+ case $TEST_SUITE in
+ actual=/tmp/gaptest.expect.actual
+ INPUTRC=/tmp/inputrc
+ expect -c 'spawn bin/gap.sh --quitonbreak -A -b  --cover coverage/testexpect.coverage' /home/runner/work/gap/gap/dev/gaptest.expect
Running test suite testexpect
spawn bin/gap.sh --quitonbreak -A -b --cover coverage/testexpect.coverage
 TIMEOUT 
Error: Process completed with exit code 2.

@fingolfin
Copy link
Member Author

Yeah, probably need to sprinkle a few sleep statements into the expect script

@fingolfin
Copy link
Member Author

fingolfin commented Feb 18, 2022

I've restarted this twice now: the first time it failed the CI / Send Slack notification on status change (push), the second time on testexpect, now again on CI / Send Slack notification on status change (push), with message:

workflow id: 3569081
Invalid $last_status: skipped
Error: Process completed with exit code 1.

Any idea what that's about?

@wilfwilson
Copy link
Member

wilfwilson commented Feb 18, 2022

I think I understand:

Since the branch of this PR is actually on this repo, then every time you push, two builds are triggered: the one for pushing a branch, and the one for updating a PR.

For some reason, the PR build is the newest one (but it is immediately skipped because it's a duplicate).

When the push CI finishes on the pushed branch, it runs the CI / Send Slack notification on status change job, which looks for the most recent CI build on the mh/tst-mock-pkg-kext branch.

It finds the build on the branch triggered by the PR update. This build was skipped.

gap-actions/should-i-notify-action does not regard skipped as a valid status: https://github.com/gap-actions/should-i-notify-action/blob/8aa058826ca437c38db4da71fdc11147fa5fd0c7/action.yml#L45 and therefore is fails.

The correct approach would be for gap-actions/should-i-notify-action to ignore builds that were triggered by pull requests. It should also gracefully accept skipped as a valid status (although this should probably never occur in practice).

@wilfwilson
Copy link
Member

I have made a change to gap-actions/should-i-notify-action that might fix things...

@fingolfin
Copy link
Member Author

Ahhh OK. Thanks for the analysis!!

Well, normally I always push branches to my repo, here I just screwed up. So I'll just ignore it.

@fingolfin
Copy link
Member Author

Thanks for the fix. Let's wait then if it works. If not, though, don't worry too much about it, we can just merge the PR anyway

@fingolfin
Copy link
Member Author

@wilfwilson seems your change did fix it, yay :-)

Copy link
Member

@wilfwilson wilfwilson left a comment

Choose a reason for hiding this comment

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

Actually, it doesn't necessarily work properly... https://github.com/gap-system/gap/runs/5254890953?check_suite_focus=true

 ┌───────┐   GAP 4.12dev built on 2022-02-18 23:39:17+0000
 │  GAP  │   https://www.gap-system.org
 └───────┘   Architecture: x86_64-pc-cygwin-default64-kv8
 Configuration:  gmp 6.2.1, GASMAN, KernelDebug
 Loading the library and packages ...
 Packages:   GAPDoc 1.6.5, PrimGrp 3.4.1, SmallGrp 1.4.2, TransGrp 3.3
 Try '??help' for help. See also '?copyright', '?cite' and '?authors'
Could not read file "/cygdrive/d/a/gap/gap/tst/mockpkg;".
Could not read file "coverage/testmockpkg.coverage".
########> Diff in /cygdrive/d/a/gap/gap/tst/mockpkg/tst/kext.tst:4
# Input is:
LoadPackage("mockpkg");
# Expected output:
true
# But found:
#I  mockpkg package is not available. Check that the name is correct
#I  and it is present in one of the GAP root directories (see '??RootPaths')
fail
########
########> Diff in /cygdrive/d/a/gap/gap/tst/mockpkg/tst/kext.tst:6
# Input is:
IsKernelExtensionAvailable("mockpkg");
# Expected output:
true
# But found:
false
########
########> Diff in /cygdrive/d/a/gap/gap/tst/mockpkg/tst/kext.tst:8
# Input is:
LoadKernelExtension("mockpkg");
# Expected output:
true
# But found:
false
########
########> Diff in /cygdrive/d/a/gap/gap/tst/mockpkg/tst/kext.tst:10
# Input is:
TestCommand();
# Expected output:
true
# But found:
Error, Variable: 'TestCommand' must have a value
########

@wilfwilson
Copy link
Member

I guess the test file needs to be run in such a way that GAP returns a non-zero exit code when it fails.

@fingolfin
Copy link
Member Author

Argh, I wanted to run tst/testall.g instead of the .tst but forgot. Thanks for paying close attention!

@fingolfin fingolfin force-pushed the mh/tst-mockpkg-kext branch 2 times, most recently from fa76518 to 57152e3 Compare February 19, 2022 00:29
@fingolfin
Copy link
Member Author

Seems to be really working now, even on Cygwin (as in: I checked the logs)

The build failure earlier was because I stupidly messed up the order of some command line arguments for gap 🙄

@fingolfin fingolfin merged commit 9076913 into master Feb 19, 2022
@fingolfin fingolfin deleted the mh/tst-mockpkg-kext branch February 19, 2022 01:14
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: ci Anything related to GitHub Actions, Codecov, AppVeyor, Coveralls, Travis, ...
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants