Skip to content
This repository was archived by the owner on Nov 24, 2025. It is now read-only.

Codecov#3768

Closed
lbathina wants to merge 10 commits intoapache:masterfrom
lbathina:codecov
Closed

Codecov#3768
lbathina wants to merge 10 commits intoapache:masterfrom
lbathina:codecov

Conversation

@lbathina
Copy link
Copy Markdown

@lbathina lbathina commented Aug 1, 2019

What does this PR (Pull Request) do?

Adds unit test coverage generation for TO and TM

  • This PR is not related to any issue

Which Traffic Control components are affected by this PR?

This doesn't affect the functionality of the any component. Just exposes the unit test coverage for TO and TM components. Testing, documentation, changelog is not required.

What is the best way to verify this PR?

Jenkins job should show coverage information by func
Screen Shot 2019-08-01 at 1 50 12 PM

The following criteria are ALL met by this PR

  • This PR includes tests OR I have explained why tests are unnecessary
  • This PR includes documentation OR I have explained why documentation is unnecessary
  • This PR includes an update to CHANGELOG.md OR such an update is not necessary
  • This PR includes any and all required license headers
  • This PR does not include a database migration
  • This PR DOES NOT FIX A SERIOUS SECURITY VULNERABILITY

@asfgit
Copy link
Copy Markdown
Contributor

asfgit commented Aug 1, 2019

Can one of the admins verify this patch?

@jhg03a
Copy link
Copy Markdown
Contributor

jhg03a commented Aug 1, 2019

ok to test

@ocket8888 ocket8888 added automation related to automated testing/deployment/packaging etc. tests related to tests and/or testing infrastructure Traffic Monitor related to Traffic Monitor Traffic Ops related to Traffic Ops labels Aug 2, 2019
Comment thread traffic_monitor/tests/Dockerfile-golangtest Outdated
Comment thread traffic_monitor/tests/Dockerfile-golangtest
Comment thread traffic_ops/app/bin/tests/Dockerfile-golangtest Outdated
Comment thread traffic_ops/app/bin/tests/Dockerfile-golangtest Outdated
Comment thread traffic_monitor/tests/gocover.bash Outdated
Comment thread traffic_monitor/tests/gocover.bash Outdated
Comment thread traffic_monitor/tests/gocover.bash Outdated
Comment thread traffic_monitor/tests/gocover.bash Outdated
@rob05c
Copy link
Copy Markdown
Member

rob05c commented Aug 19, 2019

retest this please

@asfgit
Copy link
Copy Markdown
Contributor

asfgit commented Aug 19, 2019

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/4170/
Test PASSed.

@mhoppa
Copy link
Copy Markdown
Contributor

mhoppa commented Oct 9, 2019

retest this please

2 similar comments
@mhoppa
Copy link
Copy Markdown
Contributor

mhoppa commented Oct 16, 2019

retest this please

@ocket8888
Copy link
Copy Markdown
Contributor

retest this please

@asf-ci
Copy link
Copy Markdown
Contributor

asf-ci commented Oct 16, 2019

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/4545/

@ocket8888
Copy link
Copy Markdown
Contributor

retest this please

@asf-ci
Copy link
Copy Markdown
Contributor

asf-ci commented Oct 16, 2019

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/4546/

@lbathina lbathina requested a review from ocket8888 November 15, 2019 20:33
Comment thread traffic_monitor/tests/gocover.bash Outdated
Comment thread traffic_monitor/tests/gocover.bash Outdated
Comment thread traffic_monitor/tests/gocover.bash Outdated
@ocket8888 ocket8888 self-assigned this Dec 31, 2019
tmp="$(mktemp)"
go test -v -coverprofile covProfile "$pkg" > "$tmp"
mv -f "$tmp" coverage_out1
done
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It looks like you're now building the coverage profiles twice but only reading the results once, and you're no longer writing anything into result.txt (which might be okay - is it?). My script that I posted in a comment on my earlier review wasn't meant as a verbatim example of how to accomplish what you're trying to do, merely show that the two ways of getting a coverage profile would produce equivalent results.

All you gotta do here is take that bottom for loop and paste it over the top one, I think. But it is missing some things, including a line which I missed in my comment originally (which I think is why). The actual loop should look like:

touch coverage_out
covtmp="$(mktemp)"
covMergeTmp="$(mktemp)"
for pkg in $(go list $@ | grep -v vendor); do
	tmp="$(mktemp)"
	go test -v -coverprofile "$covtmp" | tee -a result.txt
	gocovmerge coverage_out "$covtmp" > "$covMergeTmp"
	mv -f "$covMergeTmp" coverage_out
done;

which gets you what you're looking for in result.txt as well as coverage_out.
Then, of course, there's no need to initialize coverage_out as a variable (since it's a file now) or i, and the gocovmerge line can be deleted.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@ocket8888 i better leave it to you.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

abandoned automation related to automated testing/deployment/packaging etc. tests related to tests and/or testing infrastructure Traffic Monitor related to Traffic Monitor Traffic Ops related to Traffic Ops

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants