search

gap-packages / profiling

GAP profiling output
https://gap-packages.github.io/profiling/
Other
4 stars 7 forks source link

Is this package supposed to have its own code coverage? #94

Closed wilfwilson closed 2 years ago

wilfwilson commented 2 years ago

Yesterday I updated the CI to use (amongst other things) gap-actions/run-pkg-tests, at version 2 rather than version 1.

Version 1 ran the tests AND processed and the uploaded code coverage results to Codecov, whereas version 2 only runs the tests.

When changing to version 2, I forgot to add the usual additional extra steps that are now needed to process and upload the code coverage.

Therefore, as you can see at https://app.codecov.io/gh/gap-packages/profiling/branch/master/commits?page=1, coverage data is no longer being uploaded, but was being uploaded until yesterday.

However, run-pkg-tests was always run with the option NO_COVERAGE = 'yes', which means that the coverage shouldn't have been uploaded anyway.

So what do we want? Coverage or not? Is it even possible to profile the profiling package?

fingolfin commented 2 years ago

Well, it should be possible to get coverage for at least the C code, as that is generated independently from the code in this package. But IIRC it is indeed tricky to profile the profiling code itself, which is probably why NO_COVERAGE = 'yes' was used... ? One way to do that is to simply copy the code invoking gcov from the process-coverage action, i.e., find . -type f -name '*.gcno' -exec gcov -pb {} + and run it here, after the test suite has completed, and then upload via the codecov uploader action

Anyway, obviously @ChrisJefferson is the person who should decide :-)

wilfwilson commented 2 years ago

For me, the bigger question is "why was coverage being uploaded if the option NO_COVERAGE = 'yes' was being used"?

fingolfin commented 2 years ago

I think NO_COVERAGE only disabled collecting GAP profiling data, but it didn't stop it from executing the codecov uploaded, which then still found the covering data for the C code. Indeed, looking at https://app.codecov.io/gh/gap-packages/profiling there is only C code coverage

wilfwilson commented 2 years ago

Oh right, in that case I misunderstood. I had assumed NO_COVERAGE would stop doing stuff related to code coverage completely, as in the pkg-ci-scripts README:

NO_COVERAGE -- set to any value to disable coverage generation and uploading

fingolfin commented 2 years ago

pkg-ci-scripts and the actions have diverged quite a bit by now

wilfwilson commented 2 years ago

Yeah fair enough. But also, from the gap-actions/run-pkg-tests README:

  • NO_COVERAGE:
    • Set to a non-empty string to suppress gathering coverage.

So perhaps this needs clarifying?

wilfwilson commented 2 years ago

I think my confusion is being caused by what I mentioned in https://github.com/gap-actions/build-pkg/issues/6 - in essence, build-pkg is set up to always cover the C code.

wilfwilson commented 2 years ago

@ChrisJefferson confirms that, as currently implemented, the profiling package can't really be used to get code coverage of the GAP code of the profiling package. But we can still get coverage of the kernel. #91 will make things work that way (again).

My confusion with the NO_COVERAGE option in the CI is separate to this issue, and not relevant to the profiling package specifically. It is to do with the gap-actions scripts.