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

Update CI to build manual and add coverage for the server code #35

Open
2 tasks done
olexandr-konovalov opened this issue May 25, 2021 · 11 comments · Fixed by #41
Open
2 tasks done

Update CI to build manual and add coverage for the server code #35

olexandr-konovalov opened this issue May 25, 2021 · 11 comments · Fixed by #41

Comments

@olexandr-konovalov
Copy link
Member

olexandr-konovalov commented May 25, 2021

TODO:

@olexandr-konovalov olexandr-konovalov changed the title Update CI Update CI to build manual and add coverage for the server code Apr 29, 2022
@fingolfin
Copy link
Member

I had a look at reviving the server coverage tests, and had a look at the many diffs. One stood out as possibly hinting at an actual issue:

gap> t3:=KaratsubaPolynomialMultiplicationWS(f,g);;time;
Error, unexpected_symbol : cd=scscp_transient_1, name=WS_Karatsuba

@olexandr-konovalov
Copy link
Member Author

Hmm not sure that server coverage actually worked: https://app.codecov.io/gh/gap-packages/scscp/blob/master/lib/server.gi

@olexandr-konovalov
Copy link
Member Author

This does not stop me from making a new release though.

@fingolfin
Copy link
Member

Are you sure that file ever saw coverage? It is possible that GAP never writes out the profiling data, given that there are three GAP processes running here...

Although at https://github.com/gap-packages/scscp/actions/runs/3664824757/jobs/6195485192 it does seem to upload several coverage files for the various servers. Huh, weird.

Could it be that the coverage is not written out because the function RunSCSCPserver never terminates? (Instead we quit GAP via QUIT_GAP). My reading of the profiling code does not support this theory, though, it seems to write out all data immediately... Maybe @ChrisJefferson has an idea?

@olexandr-konovalov
Copy link
Member Author

I am 100% sure, yes, it worked under Travis. See #19

@fingolfin
Copy link
Member

That linked issue does not mention lib/server.gi, though, nor RunSCSCPserver. The coverage rathe went up by 31% points in #41, so it definitely does do something.

@olexandr-konovalov
Copy link
Member Author

There is a comment in its description

It also runs one of the two SCSCP servers (the one on the default port, exercised more) and creates ${COVDIR}/test-server.coverage with the coverage data from the server. It is then picked up by scripts/gather-coverage.sh and added to the coverage report, increasing it b almost 12%. This closes #15.

@fingolfin
Copy link
Member

fingolfin commented Dec 10, 2022

In #19 it says that 1652 out of 2681 lines are covered. For current master, https://app.codecov.io/gh/gap-packages/scscp reports that 1532 of 2675 lines are covered.

@fingolfin
Copy link
Member

Sorry, initial version of my previous comment was wrong, I've updated it now. There were 6 more lines in total back then, but 120 lines more covered. So there definitely was a difference, but it is hard to say where it comes from. All I can say is that the CI tests do execute RunSCSCPserver. But for reason I can't fathom right now, the coverage data for RunSCSCPserver is not collected. Weird.

@olexandr-konovalov
Copy link
Member Author

Yeah... Anyhow, it's now works good enough for the release I made, I may try to revisit this the other day...

@fingolfin
Copy link
Member

Looking at https://github.com/gap-packages/scscp/actions/runs/3664824757/jobs/6195485129 again we see in the gap-actions/process-coverage@v2 step:

Loading  profiling 2.5.1 (Line by line profiling and code coverage for GAP)
56
by Christopher Jefferson (https://caj.host.cs.st-andrews.ac.uk/).
57
Homepage: https://gap-packages.github.io/profiling/
58
Report issues at https://github.com/gap-packages/profiling/issues
59
─────────────────────────────────────────────────────────────────────────────
60
> > > gap> gap> gap> > > > gap> Merging coverage results from [ "coverage/test-server1.coverage", 
  "coverage/test-server1.coverage.10421", 
  "coverage/test-server2.coverage.10402", "coverage/test-server2.coverage", 
  "coverage/test-server1.coverage.10425", "coverage/bd5uPw.coverage", 
  "coverage/test-server1.coverage.10401", 
  "coverage/test-server1.coverage.10420" ]
gap> gap> gap> gap> Outputting JSON
+ find . -type f -name '*.gcno' -exec gcov -pb '{}' +

Those coverage/test-server1.coverage.XXX and coverage/test-server2.coverage.XXX files are created by GAP instances launched via gapd.sh. These in turn read example/myserver.g. Which as its last step executes RunSCSCPserver. We then perform tests which would fail if those servers did not run.

Perhaps profiling can not process these files for some reason? Perhaps someone needs to look at those intermediate files to see what's going on?

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 a pull request may close this issue.

2 participants