-
Notifications
You must be signed in to change notification settings - Fork 481
make: add coverage target #685
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
Conversation
How much slower does adding |
Not that much. The goal is to run make integration (CLI tests) and the other tests together in a single run, serially (which wouldn’t be ideal otherwise for CI testing) |
ffc2967
to
a843cdd
Compare
@katiewasnothere @dcantah - updated to add XCRUN so we get the css output for code coverage |
Makefile
Outdated
--ignore-filename-regex=".pb.swift" \ | ||
--ignore-filename-regex=".proto" \ | ||
--ignore-filename-regex=".grpc.swift" \ | ||
$(BUILD_BIN_DIR)/ContainerPackageTests.xctest/Contents/MacOS/ContainerPackageTests > $(COV_REPORT_FILE) ; \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we referencing xctest here? This doesn't work for me locally
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly b/c this was the path for build output after running tests (make test) on my system. Maybe we can chat through this one in realtime, or others can TAL on their platform for what's unique here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found the issue, I have my dev env setup on a case sensitive volume. The xctest bundle that shows up for me is at $(BUILD_BIN_DIR)/containerPackageTests.xctest/Contents/MacOS/containerPackageTests
so the capitalized "ContainerPackageTests" is what's causing this to fail locally for me. Can we update the PR with the lowercase containerPackageTests
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, this makes sense - will fix.
To facilitate ability to gather code coverage metrics without slowing down our integration target, let's add a coverage target. Signed-off-by: Eric Ernst <[email protected]>
To facilitate ability to gather code coverage metrics without slowing down our integration target, let's add a coverage target.
From this we can run
xcrun llvm-profdata merge
to get profdata and gather overall code coverage metricsType of Change
Motivation and Context
[Why is this change needed?]
Testing