-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Introduce coverage to PR pipelines #5357
Changes from 14 commits
1cf4094
2f68217
3a32889
41340c5
abc69fa
03364b7
c47bcce
af964b0
9f4c95f
32b865e
b7f64c4
5ef0158
ef1bb0f
7ffd7da
5b57124
b15bfc9
cc1f18c
470fff4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -46,15 +46,15 @@ local github_secret = secret('github_token', 'infra/data/ci/github/grafanabot', | |||||
// Injected in a secret because this is a public repository and having the config here would leak our environment names | ||||||
local deploy_configuration = secret('deploy_config', 'infra/data/ci/loki/deploy', 'config.json'); | ||||||
|
||||||
|
||||||
local run(name, commands) = { | ||||||
local run(name, commands, env={}) = { | ||||||
name: name, | ||||||
image: 'grafana/loki-build-image:%s' % build_image_version, | ||||||
commands: commands, | ||||||
environment: env, | ||||||
}; | ||||||
|
||||||
local make(target, container=true) = run(target, [ | ||||||
'make ' + (if !container then 'BUILD_IN_CONTAINER=false ' else '') + target, | ||||||
local make(target, container=true, args=[]) = run(target, [ | ||||||
'make ' + (if !container then 'BUILD_IN_CONTAINER=false ' else '') + target + ' ' + std.join(' ', args), | ||||||
]); | ||||||
|
||||||
local docker(arch, app) = { | ||||||
|
@@ -345,7 +345,23 @@ local manifest(apps) = pipeline('manifest') { | |||||
steps: [ | ||||||
make('check-drone-drift', container=false) { depends_on: ['clone'] }, | ||||||
make('check-generated-files', container=false) { depends_on: ['clone'] }, | ||||||
make('test', container=false) { depends_on: ['clone', 'check-generated-files'] }, | ||||||
make('test', container=false) { depends_on: ['clone'] }, | ||||||
run('clone-main', commands=['cd ..', 'git clone $CI_REPO_REMOTE loki-main', 'cd -']), | ||||||
run('test-main', commands=['cd ../loki-main', 'BUILD_IN_CONTAINER=false make test']) { depends_on: ['clone-main'] }, | ||||||
make('compare-coverage', container=false, args=[ | ||||||
'old=../loki-main/test_results.txt', | ||||||
'new=test_results.txt', | ||||||
'packages=ingester,distributor,querier,ruler,storage', | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
The ruler is not super critical here, but the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I changed the list to these: I think we can be fairly aggressive here because we ought to have color coding to help us see where to focus. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd like to also add querier/queryrange. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does it fail if the package name has changed ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We'd see a large diff, but this won't fail |
||||||
'> diff.txt', | ||||||
]) { depends_on: ['test', 'test-main'] }, | ||||||
run('report-coverage', commands=[ | ||||||
"pull=$(echo $CI_COMMIT_REF | awk -F '/' '{print $3}')", | ||||||
"body=$(jq -Rs '{body: . }' diff.txt)", | ||||||
'curl -X POST -u $USER:$TOKEN -H "Accept: application/vnd.github.v3+json" https://api.github.com/repos/grafana/loki/issues/$pull/comments -d "$body" > /dev/null', | ||||||
], env={ | ||||||
USER: 'grafanabot', | ||||||
TOKEN: { from_secret: github_secret.name }, | ||||||
}) { depends_on: ['compare-coverage'] }, | ||||||
make('lint', container=false) { depends_on: ['clone', 'check-generated-files'] }, | ||||||
make('check-mod', container=false) { depends_on: ['clone', 'test', 'lint'] }, | ||||||
{ | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,7 +10,7 @@ | |
.PHONY: validate-example-configs generate-example-config-doc check-example-config-doc | ||
.PHONY: clean clean-protos | ||
|
||
SHELL = /usr/bin/env bash | ||
SHELL = /usr/bin/env bash -o pipefail | ||
|
||
# Empty value = no -mod parameter is used. | ||
# If not empty, GOMOD is passed to -mod= parameter. | ||
|
@@ -44,7 +44,7 @@ DOCKER_IMAGE_DIRS := $(patsubst %/Dockerfile,%,$(DOCKERFILES)) | |
BUILD_IN_CONTAINER ?= true | ||
|
||
# ensure you run `make drone` after changing this | ||
BUILD_IMAGE_VERSION := 0.20.0 | ||
BUILD_IMAGE_VERSION := 0.21.0 | ||
|
||
# Docker image info | ||
IMAGE_PREFIX ?= grafana | ||
|
@@ -276,7 +276,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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You need to update the |
||
|
||
compare-coverage: | ||
./tools/diff_coverage.sh $(old) $(new) $(packages) | ||
|
||
######### | ||
# Clean # | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
#!/bin/bash | ||
|
||
if [[ ! -f "$1" ]] || [[ ! -f "$2" ]]; then | ||
echo "unable to compare test coverage: both old and new files must exist" | ||
exit 0 | ||
fi | ||
|
||
echo '```diff' | ||
for pkg in ${3//,/ }; do | ||
old=$(grep "pkg/${pkg}\s" "$1" | sed s/%// | awk '{print $5}') | ||
new=$(grep "pkg/${pkg}\s" "$2" | sed s/%// | awk '{print $5}') | ||
echo | awk -v pkg="${pkg}" -v old="${old:-0}" -v new="${new:-0}" \ | ||
'{ | ||
sign=new - old < 0 ? "-" : "+" | ||
printf ("%s %11s\t%s\n", sign, pkg, new - old) | ||
}' | ||
done | ||
echo '```' |
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.
This will get rid of the unrelated changes in
drone.yml