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

Upload code coverage #7262

Merged
merged 4 commits into from
Mar 13, 2025
Merged

Upload code coverage #7262

merged 4 commits into from
Mar 13, 2025

Conversation

stephanos
Copy link
Contributor

@stephanos stephanos commented Feb 6, 2025

What changed?

Fixed code coverage reporting.

Why?

We haven't reported code coverage in over a year.

It stopped working when we moved away from Buildkite.

How did you test it?

Checkout https://app.codecov.io/gh/temporalio/temporal

There's no data for main ("master") yet, though, until we merge.

I'll also leave the README coveralls badge there for now.

Potential risks

Documentation

Is hotfix candidate?

@stephanos stephanos force-pushed the fix-coveralls branch 30 times, most recently from de3cf47 to a7721fc Compare February 7, 2025 00:54
Comment on lines -454 to -469
.PHONY: $(SUMMARY_COVER_PROFILE)
$(SUMMARY_COVER_PROFILE):
@printf $(COLOR) "Combine coverage reports to $(SUMMARY_COVER_PROFILE)..."
@rm -f $(SUMMARY_COVER_PROFILE) $(SUMMARY_COVER_PROFILE).html
@if [ -z "$(wildcard $(TEST_OUTPUT_ROOT)/*.cover.out)" ]; then \
echo "No coverage data, aborting!" && exit 1; \
fi
@echo "mode: atomic" > $(SUMMARY_COVER_PROFILE)
$(foreach COVER_PROFILE,$(wildcard $(TEST_OUTPUT_ROOT)/*.cover.out),\
@printf "Add %s...\n" $(COVER_PROFILE); \
@grep -v -e "[Mm]ocks\?.go" -e "^mode: \w\+" $(COVER_PROFILE) >> $(SUMMARY_COVER_PROFILE) || true \
$(NEWLINE))

coverage-report: $(SUMMARY_COVER_PROFILE)
@printf $(COLOR) "Generate HTML report from $(SUMMARY_COVER_PROFILE) to $(SUMMARY_COVER_PROFILE).html..."
@go tool cover -html=$(SUMMARY_COVER_PROFILE) -o $(SUMMARY_COVER_PROFILE).html
Copy link
Contributor Author

@stephanos stephanos Mar 12, 2025

Choose a reason for hiding this comment

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

None of this is needed/used. Codecov aggregates all reports for us. And its config allows to filter out irrelevant files/folders.

@@ -60,7 +60,6 @@ env:
PR_BASE_COMMIT: ${{ github.event.pull_request.base.sha }}
DOCKER_COMPOSE_FILE: ./develop/github/docker-compose.yml
TEMPORAL_VERSION_CHECK_DISABLED: 1
BUILDKITE_ANALYTICS_TOKEN: ${{ secrets.BUILDKITE_ANALYTICS_TOKEN }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not used!

Copy link
Member

Choose a reason for hiding this comment

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

Can I delete then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes!

NEW_REPORT = $(TEST_OUTPUT_ROOT)/$(shell xxd -p -l 16 /dev/urandom).junit.xml # generates a new filename each time it's substituted
COVERPKG_FLAG = -coverpkg=$(shell go list ./... | paste -sd "," -)
Copy link
Contributor Author

@stephanos stephanos Mar 12, 2025

Choose a reason for hiding this comment

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

Just listing all packages here. Anything we don't want to be reported is filtered out by Codecov.

The only downside is that it'll generate a bunch of warning: no packages being tested depend on matches for pattern - but I think that's a small price to pay for not having to maintain this list by hand (and risking it gets out of date).

Copy link
Member

Choose a reason for hiding this comment

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

I tried to keep number of Linux commands under control but it seems to get out of it long time ago (this is comment about adding dependency in paste).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the alternative to write a Go script? I feel like there's already way too many Unix-specific stuff in this Makefile already. (e.g. /dev/urandom just above it)

@stephanos stephanos force-pushed the fix-coveralls branch 5 times, most recently from c838d73 to 5a32618 Compare March 12, 2025 20:54
NEW_COVER_PROFILE = $(TEST_OUTPUT_ROOT)/$(shell xxd -p -l 16 /dev/urandom).cover.out # generates a new filename each time it's substituted
SUMMARY_COVER_PROFILE := $(TEST_OUTPUT_ROOT)/summary.cover.out
NEW_REPORT = $(TEST_OUTPUT_ROOT)/$(shell xxd -p -l 16 /dev/urandom).junit.xml # generates a new filename each time it's substituted
NEW_COVER_PROFILE = $(TEST_OUTPUT_ROOT)/coverage.$(shell xxd -p -l 16 /dev/urandom).out # generates a new filename each time it's substituted
Copy link
Contributor Author

Choose a reason for hiding this comment

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

File name has to match any item in this list to be found by Codecov.

SUMMARY_COVER_PROFILE := $(TEST_OUTPUT_ROOT)/summary.cover.out
NEW_REPORT = $(TEST_OUTPUT_ROOT)/$(shell xxd -p -l 16 /dev/urandom).junit.xml # generates a new filename each time it's substituted
NEW_COVER_PROFILE = $(TEST_OUTPUT_ROOT)/coverage.$(shell xxd -p -l 16 /dev/urandom).out # generates a new filename each time it's substituted
NEW_REPORT = $(TEST_OUTPUT_ROOT)/junit.$(shell xxd -p -l 16 /dev/urandom).xml # generates a new filename each time it's substituted
Copy link
Contributor Author

@stephanos stephanos Mar 12, 2025

Choose a reason for hiding this comment

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

Aligning the file name pattern here with the coverage one.

I don't expect that to break anything?

Copy link
Member

Choose a reason for hiding this comment

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

We will see :-)

@temporalio temporalio deleted a comment from codecov bot Mar 12, 2025
@stephanos stephanos force-pushed the fix-coveralls branch 4 times, most recently from 0045835 to 01d652f Compare March 12, 2025 21:45
@stephanos stephanos marked this pull request as ready for review March 12, 2025 21:46
@stephanos stephanos requested a review from a team as a code owner March 12, 2025 21:46
@@ -60,7 +60,6 @@ env:
PR_BASE_COMMIT: ${{ github.event.pull_request.base.sha }}
DOCKER_COMPOSE_FILE: ./develop/github/docker-compose.yml
TEMPORAL_VERSION_CHECK_DISABLED: 1
BUILDKITE_ANALYTICS_TOKEN: ${{ secrets.BUILDKITE_ANALYTICS_TOKEN }}
Copy link
Member

Choose a reason for hiding this comment

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

Can I delete then?

SUMMARY_COVER_PROFILE := $(TEST_OUTPUT_ROOT)/summary.cover.out
NEW_REPORT = $(TEST_OUTPUT_ROOT)/$(shell xxd -p -l 16 /dev/urandom).junit.xml # generates a new filename each time it's substituted
NEW_COVER_PROFILE = $(TEST_OUTPUT_ROOT)/coverage.$(shell xxd -p -l 16 /dev/urandom).out # generates a new filename each time it's substituted
NEW_REPORT = $(TEST_OUTPUT_ROOT)/junit.$(shell xxd -p -l 16 /dev/urandom).xml # generates a new filename each time it's substituted
Copy link
Member

Choose a reason for hiding this comment

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

We will see :-)

NEW_REPORT = $(TEST_OUTPUT_ROOT)/$(shell xxd -p -l 16 /dev/urandom).junit.xml # generates a new filename each time it's substituted
COVERPKG_FLAG = -coverpkg=$(shell go list ./... | paste -sd "," -)
Copy link
Member

Choose a reason for hiding this comment

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

I tried to keep number of Linux commands under control but it seems to get out of it long time ago (this is comment about adding dependency in paste).

const (
codeCoverageExtension = ".cover.out"
retriesFlag = "--retries="
coverProfileFlag = "-coverprofile="
Copy link
Member

Choose a reason for hiding this comment

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

why this one has only one dash?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

coverprofile is from Go, --retriesFlag and --junitfile are out own invention. Not sure which one is "correct".

@stephanos stephanos merged commit b609189 into temporalio:main Mar 13, 2025
51 checks passed
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