cockpit-podman test: Collect code coverage

[x] https://github.com/cockpit-project/cockpit/pull/17078

Mar 30 '22 13:03 mvollmer

I like the report! Just looking into it I can see two things:

We define compare_versions and getCommitArr functions but not use them. We can drop this dead code
We don't test at all pod actions

Mar 30 '22 14:03 marusak

This is really cool!

Should genhtml set --title so my browser tab shows what project it generated coverage for, it would require some argument to be passed to --coverage or is there

Should we exclude src/lib? It's something which comes from cockpit so we aren't really interested in it's coverage in cockpit-podman. Does --coverage support a way to exclude data?

In the future it would be cool if we could somehow report back coverage increase / decrease in a PR? Not sure if something already exists for it?

Apr 04 '22 10:04 jelly

Should we exclude src/lib? It's something which comes from cockpit so we aren't really interested in it's coverage in cockpit-podman.

Without thinking too much about it, I would say let's keep it. it's easy enough to ignore, and might be interesting in certain cases.

May 25 '22 08:05 mvollmer

Should genhtml set --title

Yeah, let's try to make this work.

May 25 '22 08:05 mvollmer

Shall we pick this up again? As Cockpit already has the integration for a few weeks it might be nice to add it here now as well.

Sep 12 '22 12:09 jelly

Shall we pick this up again? As Cockpit already has the integration for a few weeks it might be nice to add it here now as well.

Sure. I think the blocker is still how to get a development build of cockpit-podman, no?

Sep 15 '22 13:09 mvollmer

how to get a development build of cockpit-podman

NODE_ENV=development make should works, right?

Sep 19 '22 08:09 marusak

how to get a development build of cockpit-podman

NODE_ENV=development make should works, right?

I guess, but NODE_ENV=development make check does not.

Sep 20 '22 07:09 mvollmer

@martinpitt I think the PR is ready for review now? Probably my commit should be squashed into the touching of the LICENSE file? Do you find the solution I propose acceptable?

Oct 07 '22 14:10 jelly

@mvollmer added a small comment about the LICENSE file.

Oct 24 '22 14:10 jelly

Blocked on a coverage not working without a name field in src/manifest.json.

Oct 25 '22 15:10 jelly

Eww, what? does that mean that we only get code coverage for the "systemd" webpack in cockpit?

No, cockpit uses subdirectories in dist/, and without a "name" in the manifest we can assume that the directory in the URL is the same as the directory in dist/. (We have a "name" in many manifests, not just in systemd, no?)

Starterkit-derived projects don't use subdirs in dist/, but they still have a directory in their URL, which either comes from the manifest, or is the same as their subdirectoy in /usr/share/cockpit.

Oct 26 '22 10:10 mvollmer

The coverage part of this works. I don't know what's up with the pixel test failures...

Oct 26 '22 11:10 mvollmer

The coverage part of this works. I don't know what's up with the pixel test failures...

So that's likely the bump of COCKPIT_REPO_LIB

Oct 26 '22 12:10 jelly

Thanks @mvollmer ! Pixel test failure is most likely due to the newer PF in cockpit; it could also be due to a newer browser in our tasks container, it was updated today.

Oct 26 '22 12:10 martinpitt

Same pixel changes in https://cockpit-logs.us-east-1.linodeobjects.com/pull-650-20221026-131526-25e8c1e1-fedora-36/pixeldiff.html#TestMachinesConsoles-testSerialConsole-vm-details-console-serial so I assume this is because of PF updates, @garrett do you have an idea if we should just accept this new shadow border?

Oct 27 '22 09:10 jelly

Thanks @mvollmer ! Pixel test failure is most likely due to the newer PF in cockpit; it could also be due to a newer browser in our tasks container, it was updated today.

Pixel test failure is now resolved, all green :)

Nov 03 '22 07:11 jelly

As follow up, make a PR to bots to update lib/testmap.py to add devel to our standard test set.

Nov 04 '22 14:11 jelly