Skip to content

Code Coverage Support#1380

Merged
lockshaw merged 5 commits intoflexflow:repo-refactorfrom
Bob-Chen222:code-coverage
May 14, 2024
Merged

Code Coverage Support#1380
lockshaw merged 5 commits intoflexflow:repo-refactorfrom
Bob-Chen222:code-coverage

Conversation

@Bob-Chen222
Copy link
Contributor

@Bob-Chen222 Bob-Chen222 commented Apr 21, 2024

Description of changes:

Hi @lockshaw, this is the PR for code coverage
I added code coverage support in cmake lists
My next step would be about integrating the output codecoverage into reviewable

Related Issues:

Linked Issues:

Issues closed by this PR:

  • Closes #

This change is Reviewable

@Bob-Chen222 Bob-Chen222 requested a review from lockshaw April 21, 2024 19:23
Copy link
Collaborator

@lockshaw lockshaw left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Bob-Chen222)


CMakeLists.txt line 90 at r1 (raw file):

include(nccl)
include(CodeCoverage)
append_coverage_compiler_flags()

Does this mean we're always building with coverage instrumentation?


flake.nix line 100 at r1 (raw file):

              tl-expected
              lcov # for code coverage
              xdg_utils # for xdg-open to open html files

Use the version from the system instead. If xdg-open isn't present then proj can just shout at the user. It makes more sense to have the external system configure it since the whole point is to allow the user to configure which browser to open on their system

@Bob-Chen222
Copy link
Contributor Author

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Bob-Chen222)

CMakeLists.txt line 90 at r1 (raw file):

include(nccl)
include(CodeCoverage)
append_coverage_compiler_flags()

Does this mean we're always building with coverage instrumentation?

flake.nix line 100 at r1 (raw file):

              tl-expected
              lcov # for code coverage
              xdg_utils # for xdg-open to open html files

Use the version from the system instead. If xdg-open isn't present then proj can just shout at the user. It makes more sense to have the external system configure it since the whole point is to allow the user to configure which browser to open on their system

Yes, it is currently always built with code coverage. I can add an optional flag for the build command so that we can choose whether to include code coverage. Do you think this would work?

@lockshaw
Copy link
Collaborator

@Bob-Chen222 Let's implement it in a separate PR--I'd like to get this merged.

It would be best for it to be bundled into a command, like proj test --coverage or something like that so users don't have to explicitly think about how stuff is built, etc. We can even have two build directories to avoid rebuilds. Can you create an issue and we can discuss it further there?

Also, reviewable is preferred over github comments where possible 🙂

Copy link
Contributor Author

@Bob-Chen222 Bob-Chen222 left a comment

Choose a reason for hiding this comment

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

Done

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @lockshaw)


CMakeLists.txt line 90 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Does this mean we're always building with coverage instrumentation?

created a new issue for this discussion


flake.nix line 100 at r1 (raw file):

Previously, Bob-Chen222 (Bob Chen) wrote…

Yes, it is currently always built with code coverage. I can add an optional flag for the build command so that we can choose whether to include code coverage. Do you think this would work?

Just deleted the xdg-open from flake.nix

@Bob-Chen222 Bob-Chen222 requested a review from lockshaw May 14, 2024 19:32
Copy link
Collaborator

@lockshaw lockshaw left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Bob-Chen222)

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 this pull request may close these issues.

3 participants