Introduce coverage to PR pipelines #5357

MasslessParticle · 2022-02-09T20:40:26Z

This PR is part one of two to introduce test coverage to our CI pipeline.

The strategy is to:

run tests against the PR and main seperately
each test run drops a test_results.txt file
the script diff_coverage will compare the results files and produce a coverage diff for the ingester, distributor, querier, ruler, and storage packages.
post the diff to the open PR as an issue.

This PR is for steps 1 - 3. Because we need all branches to produce a test_results.txt file, this is a no-op until merged into main. Noop output: https://drone.grafana.net/grafana/loki/8632/1/6

Note on coverage.txt: The tests already produce a test profile but it only produces per-function results and the go tool coverage -html is currently broken so it was more sraight forward to search the tests results for package-level results dircetly.

dannykopping

Looks good, but have a couple questions.
PS: once #5368 is merged you should merge in main to get rid of the unrelated changes to .drone.yml

dannykopping · 2022-02-11T07:16:44Z

tools/diff_coverage.sh

+fi
+
+echo '```diff'
+for pkg in ingester distributor querier ruler storage; do


Can we make this list configurable in drone? I'd like to surface that there for clarity. This list would need to be amended, but is a pretty good start

What's the reason to only calculate the diff for these packages? I think it would make sense to show all packages.

Also, did you research if there are existing tools to calculate/display coverage diffs based on go test output?

think all packages could be overwhelming. We want to keep the signal-to-noise ratio high/useful

Although, this is using diff format so it might be enough to look for red lines.

I looked a couple of drone tools but the idea here was:

something as minimal as possible

something we control -- we've had security issues with other tools

The next step of this is to make a github comment from a drone job with the diff

.drone/drone.jsonnet

chaudum · 2022-02-16T07:37:42Z

Makefile

@@ -274,7 +274,10 @@ lint:
 ########

 test: all
-	GOGC=10 $(GOTEST) -covermode=atomic -coverprofile=coverage.txt $(MOD_FLAG) -p=4 ./...
+	GOGC=10 $(GOTEST) -covermode=atomic -coverprofile=coverage.txt $(MOD_FLAG) -p=4 ./... | tee test_results.txt


You need to update the SHELL variable in the Makefile to use bash -o pipefail
Otherwise the exit code from go test is not propagated. I ran into the same issue once :D

chaudum · 2022-02-16T07:39:21Z

Makefile

+	GOGC=10 $(GOTEST) -covermode=atomic -coverprofile=coverage.txt $(MOD_FLAG) -p=4 ./... | tee test_results.txt
+
+compare-coverage:
+	./tools/diff_coverage.sh $(old) $(new)


I might miss something, but where are old and new declared?

These are declared as part of the make run: make compare-coverage old=... new=...

This part can't be implemented until we have something to compare with

chaudum · 2022-02-16T09:44:25Z

tools/diff_coverage.sh

+fi
+
+echo '```diff'
+for pkg in ingester distributor querier ruler storage; do


What's the reason to only calculate the diff for these packages? I think it would make sense to show all packages.

chaudum · 2022-02-16T09:45:50Z

tools/diff_coverage.sh

+fi
+
+echo '```diff'
+for pkg in ingester distributor querier ruler storage; do


Also, did you research if there are existing tools to calculate/display coverage diffs based on go test output?

grafanabot · 2022-02-16T22:50:56Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,ruler,storage
unable to compare test coverage: both old and new files must exist

MasslessParticle · 2022-02-16T22:54:07Z

@dannykopping I realized there was no need to wait and implement the comment so I did it today. There's nothing interesting, yet, but future prs should have output like:

./tools/diff_coverage.sh ../tloki/results.txt test_results.txt ruler,ingester,querier

+       ruler	0
+    ingester	0
+     querier	0

dannykopping

Looks great @MasslessParticle! I've added two suggestions, and then LGTM

dannykopping · 2022-02-17T06:20:25Z

.drone/drone.jsonnet

+      make('compare-coverage', container=false, args=[
+        'old=../loki-main/test_results.txt',
+        'new=test_results.txt',
+        'packages=ingester,distributor,querier,ruler,storage',


Suggested change

'packages=ingester,distributor,querier,ruler,storage',

'packages=ingester,distributor,querier,iter,storage',

The ruler is not super critical here, but the iter package has far-reaching consequences.
Maybe @cyriltovena can suggest any other critical packages we need to cover if there are any?

I changed the list to these: ingester,distributor,querier,iter,storage,chunkenc,logql,loki

I think we can be fairly aggressive here because we ought to have color coding to help us see where to focus.

I'd like to also add querier/queryrange.

does it fail if the package name has changed ?

We'd see a large diff, but this won't fail

dannykopping · 2022-02-17T06:48:23Z

.drone/drone.jsonnet

+local make(target, container=true, args=[]) = run(target, [
+  'make ' + (if !container then 'BUILD_IN_CONTAINER=false ' else '') + target + ' ' + std.join(' ', args),


Suggested change

local make(target, container=true, args=[]) = run(target, [

'make ' + (if !container then 'BUILD_IN_CONTAINER=false ' else '') + target + ' ' + std.join(' ', args),

local make(target, container=true, args=[]) = run(target, [

std.join(' ', [

'make',

'BUILD_IN_CONTAINER=' + container,

target,

] + args),

This will get rid of the unrelated changes in drone.yml

grafanabot · 2022-02-18T14:45:55Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,iter,storage,chunkenc,logql,loki
unable to compare test coverage: both old and new files must exist

dannykopping

LGTM, thanks @MasslessParticle! Excited for this

grafanabot · 2022-03-02T16:31:13Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki
unable to compare test coverage: both old and new files must exist

stale · 2022-04-16T03:14:48Z

Hi! This issue has been automatically marked as stale because it has not had any
activity in the past 30 days.

We use a stalebot among other tools to help manage the state of issues in this project.
A stalebot can be very useful in closing issues in a number of cases; the most common
is closing issues or PRs where the original reporter has not responded.

Stalebots are also emotionless and cruel and can close issues which are still very relevant.

If this issue is important to you, please add a comment to keep it open. More importantly, please add a thumbs-up to the original issue entry.

We regularly sort for closed issues which have a stale label sorted by thumbs up.

We may also:

Mark issues as revivable if we think it's a valid issue but isn't something we are likely
to prioritize in the future (the issue will still remain closed).
Add a keepalive label to silence the stalebot if the issue is very common/popular/important.

We are doing our best to respond, organize, and prioritize all issues but it can be a challenging task,
our sincere apologies if you find yourself at the mercy of the stalebot.

dannykopping · 2022-05-04T04:11:55Z

@MasslessParticle following up on this: are we going to land this one?

MasslessParticle · 2022-05-04T14:23:19Z

@dannykopping I think I've addressed all the feedback? afaik, this is ready to merge.

grafanabot · 2022-05-04T14:36:30Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki
unable to compare test coverage: both old and new files must exist

pull-request-size bot added the size/S label Feb 9, 2022

MasslessParticle closed this Feb 9, 2022

MasslessParticle reopened this Feb 9, 2022

pull-request-size bot added size/M and removed size/S labels Feb 9, 2022

MasslessParticle force-pushed the tpatterson/add-coverage branch from 9c5f0e2 to b6cdaa4 Compare February 10, 2022 17:33

MasslessParticle changed the title ~~Test CI Changes~~ Introduce coverage to PR pipelines Feb 10, 2022

MasslessParticle marked this pull request as ready for review February 10, 2022 18:10

MasslessParticle requested a review from a team as a code owner February 10, 2022 18:10

MasslessParticle requested review from cyriltovena and dannykopping February 10, 2022 18:10

dannykopping reviewed Feb 11, 2022

View reviewed changes

chaudum reviewed Feb 16, 2022

View reviewed changes

MasslessParticle added 3 commits February 16, 2022 11:30

Add code coverage diff to CI

1cf4094

remove test package from coverage diff

2f68217

review feedback

3a32889

MasslessParticle force-pushed the tpatterson/add-coverage branch from e155628 to 3a32889 Compare February 16, 2022 20:18

MasslessParticle added 2 commits February 16, 2022 13:22

lint

41340c5

Env variable test

abc69fa

pull-request-size bot added size/L and removed size/M labels Feb 16, 2022

MasslessParticle added 7 commits February 16, 2022 14:02

faster experiment

03364b7

drone vars

c47bcce

drone vars

af964b0

how does the shell work?

9f4c95f

add pr comment

32b865e

escape json

b7f64c4

report diff in comment

5ef0158

grafana deleted a comment from grafanabot Feb 16, 2022

properly format json

ef1bb0f

quote post body

7ffd7da

MasslessParticle requested review from dannykopping and chaudum February 16, 2022 22:58

dannykopping reviewed Feb 17, 2022

View reviewed changes

review feedback

5b57124

dannykopping approved these changes Mar 2, 2022

View reviewed changes

MasslessParticle added 2 commits March 2, 2022 09:19

Merge branch 'main' into tpatterson/add-coverage

b15bfc9

add querier/queryrange to coverage

cc1f18c

fridgepoet mentioned this pull request Mar 23, 2022

Add test coverage github action grafana/timestream-datasource#151

Merged

3 tasks

stale bot added the stale A stale issue or PR that will automatically be closed. label Apr 16, 2022

stale bot closed this Apr 25, 2022

MasslessParticle reopened this Apr 27, 2022

stale bot removed the stale A stale issue or PR that will automatically be closed. label Apr 27, 2022

Merge branch 'main' into tpatterson/add-coverage

470fff4

owen-d merged commit d97724e into grafana:main May 4, 2022

This was referenced Jun 28, 2022

dannykopping/remove cache stats dannykopping/loki#13

Closed

dannykopping/remove cache stats dannykopping/loki#14

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce coverage to PR pipelines #5357

Introduce coverage to PR pipelines #5357

MasslessParticle commented Feb 9, 2022 •

edited

Loading

dannykopping left a comment

dannykopping Feb 11, 2022

chaudum Feb 16, 2022

chaudum Feb 16, 2022

MasslessParticle Feb 16, 2022

chaudum Feb 16, 2022

chaudum Feb 16, 2022

MasslessParticle Feb 16, 2022

chaudum Feb 16, 2022

chaudum Feb 16, 2022

grafanabot commented Feb 16, 2022

MasslessParticle commented Feb 16, 2022

dannykopping left a comment

dannykopping Feb 17, 2022

MasslessParticle Feb 17, 2022

cyriltovena Mar 2, 2022

cyriltovena Mar 2, 2022

MasslessParticle Mar 2, 2022

dannykopping Feb 17, 2022

grafanabot commented Feb 18, 2022

dannykopping left a comment

grafanabot commented Mar 2, 2022

stale bot commented Apr 16, 2022

dannykopping commented May 4, 2022

MasslessParticle commented May 4, 2022

grafanabot commented May 4, 2022

	'packages=ingester,distributor,querier,ruler,storage',
	'packages=ingester,distributor,querier,iter,storage',

		local make(target, container=true, args=[]) = run(target, [
		'make ' + (if !container then 'BUILD_IN_CONTAINER=false ' else '') + target + ' ' + std.join(' ', args),

Introduce coverage to PR pipelines #5357

Introduce coverage to PR pipelines #5357

Conversation

MasslessParticle commented Feb 9, 2022 • edited Loading

dannykopping left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grafanabot commented Feb 16, 2022

MasslessParticle commented Feb 16, 2022

dannykopping left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grafanabot commented Feb 18, 2022

dannykopping left a comment

Choose a reason for hiding this comment

grafanabot commented Mar 2, 2022

stale bot commented Apr 16, 2022

dannykopping commented May 4, 2022

MasslessParticle commented May 4, 2022

grafanabot commented May 4, 2022

MasslessParticle commented Feb 9, 2022 •

edited

Loading