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

Travis: run testinstall with all packages after testpackages #2835

Merged
merged 1 commit into from
Feb 6, 2019

Conversation

fingolfin
Copy link
Member

No description provided.

@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 Sep 19, 2018
@fingolfin fingolfin added the gapdays2018-fall Issues and PRs that arose at https://www.gapdays.de/gapdays2018-fall label Sep 19, 2018
Copy link
Member

@olexandr-konovalov olexandr-konovalov left a comment

Choose a reason for hiding this comment

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

Nice! Of course, test with all packages fails now, but will hopefully be fixed by #2815. Perhaps you can cherry-pick this commit to the branch used for #2815, it will help to see it's effect, and then remove if from there before merging, so that we still will have two separate PRs.

I am going to update package archives tonight, if tests will go well, to give you latest Semigroups and some other updates since last week.

@codecov
Copy link

codecov bot commented Sep 21, 2018

Codecov Report

Merging #2835 into master will decrease coverage by 12.64%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master    #2835       +/-   ##
===========================================
- Coverage   85.08%   72.43%   -12.65%     
===========================================
  Files         695      628       -67     
  Lines      344362   314874    -29488     
===========================================
- Hits       293002   228094    -64908     
- Misses      51360    86780    +35420
Impacted Files Coverage Δ
lib/contfrac.gi 5.08% <0%> (-93.49%) ⬇️
lib/helpt2t.gi 0.23% <0%> (-88.25%) ⬇️
lib/teachm2.g 15.38% <0%> (-84.62%) ⬇️
lib/ctbllatt.gi 0.81% <0%> (-83.76%) ⬇️
lib/proto.gi 1.03% <0%> (-83.51%) ⬇️
src/compiler.c 8.15% <0%> (-80.92%) ⬇️
lib/ctblauto.gi 5.98% <0%> (-78.04%) ⬇️
lib/algliess.gi 0.99% <0%> (-76.96%) ⬇️
lib/ctblpope.gi 1.77% <0%> (-75.71%) ⬇️
lib/attr.gi 27.27% <0%> (-72.73%) ⬇️
... and 660 more

@olexandr-konovalov
Copy link
Member

@fingolfin please rebase this to see how the fix from #2815 will fare.

@fingolfin
Copy link
Member Author

fingolfin commented Sep 24, 2018

I already rebased 2 days ago

@fingolfin
Copy link
Member Author

The build log from 2 days ago had lots of errors I cannot reproduce locally. So I have rebased this again now; and I'll also re-run local tests, with a pristine pkg obtained via make boostrap-pkg-full (to make sure I see the exact same package versions as are used on Travis; I currently use a package distribution tarball from a few days ago, with a few very minor fixes applied; while I don't see how those could mask the errors reported on Travis, let's try to exclude all and any variation)

@fingolfin
Copy link
Member Author

Ahhh, of course -- most of these errors are due to gap-packages/JupyterKernel#77 -- but @ssiccha and @markuspf made a new release yesterday, so I hope that once this is picked up by the package distribution, we are down to the handful of errors I am seeing locally.

@fingolfin
Copy link
Member Author

@alex-konovalov Tests now pass here. However, I found another serious bug in the method reordering code that surfaces in teststandard. Once I fixed that, I'll make a PR with that fix, and the other fixes in here.

@ChrisJefferson
Copy link
Contributor

This is currently passing. Would it pass if I merged it right now?

@olexandr-konovalov olexandr-konovalov added the do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) label Sep 28, 2018
@olexandr-konovalov
Copy link
Member

@ChrisJefferson no - It should not pass because of the remaining issues after the method reordering (see #2818). This PR passes now only because it has 3 commits instead of one. As far as I understand, this is WiP, and before merge one of the commits should be removed, and another should be moved to a different PR. I've added the "don't merge" label to make it clear.

@olexandr-konovalov
Copy link
Member

@fingolfin please rebase this after #2869 has been merged, and remove the hack which disables some tests.

@olexandr-konovalov
Copy link
Member

Two tests currently fail because of the broken XML:

XML Parse Error: Line 8273 Character 1
Original file: /home/travis/build/gap-system/gap/doc/ref/../../lib/test.gi, li\
ne number 280.
-----------
</List>
^
-----------
wrong end tag, expecting "</Description>" (starts line 8171)
!!! Type 'Show();' to watch the input string in pager - starting with
    line containing error !!!

@olexandr-konovalov
Copy link
Member

@fingolfin The problem with building manuals is not present in the master branch. Please rebase.

@fingolfin fingolfin force-pushed the mh/testinstall-loadall branch 3 times, most recently from 9026d4d to 7437084 Compare October 9, 2018 14:11
@fingolfin
Copy link
Member Author

I've rebased this now, and it there is on reported test failure before the tests hang for more than 10 minutes in tst/testinstall/opers/MaximalNormalSubgroups.tst, on or after line 32. The one test failure that is reported before this is in tst/testinstall/opers/IsSolvableGroup.tst:15:

# Input is:
IsSolvable(DihedralGroup(IsFpGroup,24));
# Expected output:
true
# But found:
Error, Record Element: '<rec>.absolutelyIrreducible' must have an assigned value

Looking closer at the hang, it might actually be related to the above test failure.

@fingolfin fingolfin added topic: ci Anything related to GitHub Actions, Codecov, AppVeyor, Coveralls, Travis, ... and removed topic: tests issues or PRs related to tests labels Jan 16, 2019
@olexandr-konovalov
Copy link
Member

@fingolfin can we try again this?

@fingolfin
Copy link
Member Author

Rebased

@fingolfin
Copy link
Member Author

The 64 bit version went fine through. For the ABI=32 job, all tests also passed. But for some reasons I cannot fathom right now, it failed in the coverage gathering step, see https://api.travis-ci.org/v3/job/485322346/log.txt resp. here:

> > > gap> gap> Scanning for coverage data...
gap> gap> > > > > >   coverage/testinstall-loadall.coverage
gap> > Merging 1 coverage files...
> Error, Unable to open file coverage/testinstall-loadall.coverage in
  READ_PROFILE_FROM_STREAM( UserHomeExpand( filename ), 0 
 ) at /home/travis/build/gap-system/gap/pkg/profiling/gap/profiling.gi:12 called from 
ReadLineByLineProfile( f 
 ) at /home/travis/build/gap-system/gap/pkg/profiling/gap/profiling.gi:120 called from
<function "MergeLineByLineProfiles">( <arguments> )
 called from read-eval loop at *stdin*:16

Perhaps @ChrisJefferson has an idea?

For now, I've added a change to show the content of the coverage dir just before the gathering step (I'd leave that change in anyway, we've had problems at the point before).

@fingolfin
Copy link
Member Author

So I added that ls -l, and am seeing this:

+COVDIR=coverage
+ls -l coverage
total 2234212
-rw-rw-r-- 1 travis travis 2287828466 Jan 28 17:31 testinstall-loadall.coverage
+bin/gap.sh --quitonbreak --alwaystrace -q -a 500M -m 500M -q

So it seems we got a coverage file with > 2 GB data in it. My guess would be that the 32 bit version of GAP and profiling just are not able to handle that.

Hence one option would be to simply apply this change only to the 64 bit version, but not the 32 bit version. That still serves our purposes, I think, and side steps the issue at hand. Thoughts? @ChrisJefferson @alex-konovalov ?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 84.998% when pulling 027760a on fingolfin:mh/testinstall-loadall into dd34ea2 on gap-system:master.

@ChrisJefferson
Copy link
Contributor

I think that's best. I'm quite suprised that coverage file loads in a reasonable time even in 64-bit GAP, but if it's working that's fine. Also, coverage files don't normally get that large, because they should only include each line once...

@olexandr-konovalov
Copy link
Member

Happy with 64-bit testing only. Shall we remove "do not merge" label?

@fingolfin fingolfin removed the do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) label Jan 29, 2019
@gap-system gap-system deleted a comment from coveralls Jan 29, 2019
@fingolfin fingolfin merged commit ef9c0d1 into gap-system:master Feb 6, 2019
@fingolfin fingolfin deleted the mh/testinstall-loadall branch February 6, 2019 23:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gapdays2018-fall Issues and PRs that arose at https://www.gapdays.de/gapdays2018-fall 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.

4 participants