[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #3768: Codecov
ocket commented on a change in pull request #3768: Codecov
URL: https://github.com/apache/trafficcontrol/pull/3768#discussion_r362959470
##
File path: traffic_monitor/tests/gocover.bash
##
@@ -0,0 +1,38 @@
+#!/usr/bin/env bash
+
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+#
+set -e
+
+coverage_out=()
+i=1
+
+for d in $(go list ./... ../lib/... | grep -v vendor); do
+ file="$i.out"
+ go test -v -coverprofile="$file" "$d"
+ if [ -f "$file" ]; then
+ coverage_out+=($file)
+ fi
+ ((i++))
+done
+
+gocovmerge ${coverage_out[*]} > coverage.out
+go tool cover -func=coverage.out
+go-junit-report --package-name=golang.test.tm --set-exit-code >
/junit/golang.test.tm.xml
+touch coverage_out1
+for pkg in $(go list ./... ../lib/... | grep -v ); do
+ tmp="$(mktemp)"
+ go test -v -coverprofile covProfile "$pkg" > "$tmp"
+ mv -f "$tmp" coverage_out1
+done
Review comment:
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:
```bash
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.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #3768: Codecov
ocket commented on a change in pull request #3768: Codecov
URL: https://github.com/apache/trafficcontrol/pull/3768#discussion_r349699939
##
File path: traffic_monitor/tests/gocover.bash
##
@@ -0,0 +1,36 @@
+#!/usr/bin/env bash
+
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+#
+set -e
+go get "github.com/wadey/gocovmerge"
+touch result.txt
+packages=( "$@" )
+coverage_out=()
+i=1
+for pkg in ${packages[@]} ; do
+for d in $(go list $pkg | grep -v vendor); do
+file="$i.out"
+go test -v -coverprofile=$file $d | tee -a result.txt
+cat result.txt
+if [ -f $file ]; then
+coverage_out+=( $file )
+fi
+((i++))
Review comment:
Yes it does:
```bash
#!/usr/bin/bash
# $GOPATH/src/github.com/apache/trafficcontrol/traffic_monitor/gotestcov.sh
coverage_out=()
i=1
for d in $(go list ./... ../lib/... | grep -v vendor); do
file="$i.out"
go test -v -coverprofile="$file" "$d"
if [ -f "$file" ]; then
coverage_out+=($file)
fi
((i++))
done
gocovmerge ${coverage_out[*]} > coverage.out
touch coverage_out
for pkg in $(go list ./... ../lib/... | grep -v ); do
tmp="$(mktemp)"
go test -v -coverprofile covProfile "$pkg" > "$tmp"
mv -f "$tmp" coverage_out
done
```
then:
```shell
$ chmod a+x ./gotestcov.sh
$ ./gotestcov.sh > /dev/null
$ diff coverage_out coverage.out
$ wc -l coverage.out
3454 coverage.out
```
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #3768: Codecov
ocket commented on a change in pull request #3768: Codecov
URL: https://github.com/apache/trafficcontrol/pull/3768#discussion_r349679030
##
File path: traffic_monitor/tests/gocover.bash
##
@@ -0,0 +1,36 @@
+#!/usr/bin/env bash
+
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+#
+set -e
+go get "github.com/wadey/gocovmerge"
+touch result.txt
+packages=( "$@" )
+coverage_out=()
+i=1
+for pkg in ${packages[@]} ; do
+for d in $(go list $pkg | grep -v vendor); do
+file="$i.out"
+go test -v -coverprofile=$file $d | tee -a result.txt
+cat result.txt
+if [ -f $file ]; then
+coverage_out+=( $file )
+fi
+((i++))
Review comment:
Yes, besides that fact that `${@[@]}` should just be `$@` you'd still need a
temporary file to avoid clobbering yourself. Could do
```bash
tmp="$(mktemp)"
gocovmerge coverage_out "$file" > "$tmp"
mv -f "$tmp" ./coverage_out
```
Which isn't a *whole* lot better - although it is just one extra file per
package instead of N for N packages -, so if you don't think it's worth
rewriting for that I'm good leaving it as-is.
This'd be a lot easier if `gocovmerge` could take input on stdin.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #3768: Codecov
ocket commented on a change in pull request #3768: Codecov
URL: https://github.com/apache/trafficcontrol/pull/3768#discussion_r347025411
##
File path: traffic_monitor/tests/gocover.bash
##
@@ -0,0 +1,36 @@
+#!/usr/bin/env bash
+
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+#
+set -e
+go get "github.com/wadey/gocovmerge"
+touch result.txt
+packages=( "$@" )
+coverage_out=()
+i=1
+for pkg in ${packages[@]} ; do
+for d in $(go list $pkg | grep -v vendor); do
+file="$i.out"
+go test -v -coverprofile=$file $d | tee -a result.txt
+cat result.txt
+if [ -f $file ]; then
+coverage_out+=( $file )
+fi
+((i++))
Review comment:
creating all of these files is a bit messy and confusing. Can't you do this
with one (properly) temporary file like:
```bash
set -ex
go get github.com/wadey/gocovmerge
touch coverage_out
file="$(mktemp)"
for pkg in ${@[@]}; do
for d in $(go list $pkg | grep -v vendor); do
go test -v -coverprofile "$file" "$d" | tee -a result
gocovmerge coverage_out "$file" > coverage_out
done;
done;
go tool cover -func=coverage_out
go-junit-report --package-name=golang.test.tm --set-exit-code < result | tee
/junit/golang.test.tm.xml
```
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #3768: Codecov
ocket commented on a change in pull request #3768: Codecov URL: https://github.com/apache/trafficcontrol/pull/3768#discussion_r347064327 ## File path: traffic_monitor/tests/gocover.bash ## @@ -0,0 +1,36 @@ +#!/usr/bin/env bash + +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# +set -e +go get "github.com/wadey/gocovmerge" Review comment: How come you can't do this in the dockerfile when you're `go get`-ing the other stuff you need? That way a built image could be used even offline. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #3768: Codecov
ocket commented on a change in pull request #3768: Codecov
URL: https://github.com/apache/trafficcontrol/pull/3768#discussion_r347020709
##
File path: traffic_monitor/tests/gocover.bash
##
@@ -0,0 +1,36 @@
+#!/usr/bin/env bash
+
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+#
+set -e
+go get "github.com/wadey/gocovmerge"
+echo "" > result.txt
+packages=( "$@" )
+coverage_out=()
+i=1
+for pkg in ${packages[@]} ; do
+for d in $(go list $pkg | grep -v vendor); do
+file="$i.out"
+go test -v -coverprofile=$file $d >> result.txt
+cat result.txt
Review comment:
You did the `tee` but you still have this `cat` here, making it even more
suplerfuous
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #3768: Codecov
ocket commented on a change in pull request #3768: Codecov
URL: https://github.com/apache/trafficcontrol/pull/3768#discussion_r314407957
##
File path: traffic_monitor/tests/gocover.bash
##
@@ -0,0 +1,36 @@
+#!/usr/bin/env bash
+
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+#
+set -e
+go get "github.com/wadey/gocovmerge"
+echo "" > result.txt
+packages=( "$@" )
+coverage_out=()
+i=1
+for pkg in ${packages[@]} ; do
+for d in $(go list $pkg | grep -v vendor); do
+file="$i.out"
+go test -v -coverprofile=$file $d >> result.txt
+cat result.txt
+if [ -f $file ]; then
+coverage_out+=( $file )
+fi
+((i++))
+done
+done
+gocovmerge ${coverage_out[*]} > coverage.out
+go tool cover -func=coverage.out
+cat result.txt | go-junit-report --package-name=golang.test.tm --set-exit-code
> /junit/golang.test.tm.xml
+chmod 777 -R /junit && cat /junit/golang.test.tm.xml
Review comment:
You're probably right. It might be worth checking, though, if everything
works if you delete everything before `cat` on that line.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #3768: Codecov
ocket commented on a change in pull request #3768: Codecov URL: https://github.com/apache/trafficcontrol/pull/3768#discussion_r314406675 ## File path: traffic_monitor/tests/gocover.bash ## @@ -0,0 +1,36 @@ +#!/usr/bin/env bash + +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# +set -e +go get "github.com/wadey/gocovmerge" Review comment: You can ask the mailing list about using this library in this way. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #3768: Codecov
ocket commented on a change in pull request #3768: Codecov URL: https://github.com/apache/trafficcontrol/pull/3768#discussion_r314406505 ## File path: traffic_monitor/tests/gocover.bash ## @@ -0,0 +1,36 @@ +#!/usr/bin/env bash + +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# +set -e +go get "github.com/wadey/gocovmerge" +echo "" > result.txt Review comment: lol no This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #3768: Codecov
ocket commented on a change in pull request #3768: Codecov URL: https://github.com/apache/trafficcontrol/pull/3768#discussion_r313633295 ## File path: traffic_monitor/tests/gocover.bash ## @@ -0,0 +1,36 @@ +#!/usr/bin/env bash + +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# +set -e +go get "github.com/wadey/gocovmerge" +echo "" > result.txt Review comment: You don't need to change this, but for future reference I think you're looking for [`touch(1)`](https://linux.die.net/man/1/touch). This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #3768: Codecov
ocket commented on a change in pull request #3768: Codecov
URL: https://github.com/apache/trafficcontrol/pull/3768#discussion_r313635588
##
File path: traffic_monitor/tests/gocover.bash
##
@@ -0,0 +1,36 @@
+#!/usr/bin/env bash
+
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+#
+set -e
+go get "github.com/wadey/gocovmerge"
+echo "" > result.txt
+packages=( "$@" )
+coverage_out=()
+i=1
+for pkg in ${packages[@]} ; do
+for d in $(go list $pkg | grep -v vendor); do
+file="$i.out"
+go test -v -coverprofile=$file $d >> result.txt
+cat result.txt
+if [ -f $file ]; then
+coverage_out+=( $file )
+fi
+((i++))
+done
+done
+gocovmerge ${coverage_out[*]} > coverage.out
+go tool cover -func=coverage.out
+cat result.txt | go-junit-report --package-name=golang.test.tm --set-exit-code
> /junit/golang.test.tm.xml
+chmod 777 -R /junit && cat /junit/golang.test.tm.xml
Review comment:
Why is this chmod necessary? I know this is running in a container, but I
still balk a little at any time I see a `chmod` with more permissions than it
needs - especially with `-R`.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #3768: Codecov
ocket commented on a change in pull request #3768: Codecov URL: https://github.com/apache/trafficcontrol/pull/3768#discussion_r313632408 ## File path: traffic_ops/app/bin/tests/Dockerfile-golangtest ## @@ -18,8 +18,13 @@ ARG DIR=github.com/apache/trafficcontrol ADD traffic_ops /go/src/$DIR/traffic_ops ADD lib /go/src/$DIR/lib +COPY traffic_monitor/tests/gocover.bash /go/src/$DIR/traffic_ops/traffic_ops_golang/gocover.bash +RUN chmod +x /go/src/$DIR/traffic_ops/traffic_ops_golang/gocover.bash WORKDIR /go/src/$DIR/traffic_ops/traffic_ops_golang +RUN bash -c "go get -u github.com/jstemmer/go-junit-report" +RUN bash -c "go get -v" Review comment: You're not using any extended features of `bash` here, so there's no reason to force the fork to a shell that may not be in the `PATH` if the image's base is changed (unlikely though that is). You can do all of this in one step after the `WORKDIR` directive to reduce caching as well: ```Dockerfile WORKDIR /go/src/$DIR/traffic_ops/traffic_ops_golang RUN chmod a+x gocover.bash && \ go get -u github.com/jstemmer/go-junit-report && \ go get -v ``` This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #3768: Codecov
ocket commented on a change in pull request #3768: Codecov URL: https://github.com/apache/trafficcontrol/pull/3768#discussion_r313633026 ## File path: traffic_monitor/tests/gocover.bash ## @@ -0,0 +1,36 @@ +#!/usr/bin/env bash + +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# +set -e +go get "github.com/wadey/gocovmerge" Review comment: You should be fetching dependencies that exist for the image's entire run-time during the build step. But also, has this dependency been approved on a license basis? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #3768: Codecov
ocket commented on a change in pull request #3768: Codecov
URL: https://github.com/apache/trafficcontrol/pull/3768#discussion_r313635034
##
File path: traffic_monitor/tests/gocover.bash
##
@@ -0,0 +1,36 @@
+#!/usr/bin/env bash
+
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+#
+set -e
+go get "github.com/wadey/gocovmerge"
+echo "" > result.txt
+packages=( "$@" )
+coverage_out=()
+i=1
+for pkg in ${packages[@]} ; do
+for d in $(go list $pkg | grep -v vendor); do
+file="$i.out"
+go test -v -coverprofile=$file $d >> result.txt
+cat result.txt
Review comment:
So now, for every iteration of each loop you're printing the entire results
file, which means e.g. you're going to see the first line again on every
iteration. Correct me if I'm wrong, but I think what would be better is to
print out each line as you go. To do that, you can use
[`tee(1)`](http://man7.org/linux/man-pages/man1/tee.1.html) like so:
```bash
go test -v --coverprofile "$file" "$d" | tee -a result.txt # note the quotes
around shell variables to prevent interpretation
```
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #3768: Codecov
ocket commented on a change in pull request #3768: Codecov URL: https://github.com/apache/trafficcontrol/pull/3768#discussion_r310636896 ## File path: traffic_monitor/tests/Dockerfile-golangtest ## @@ -23,6 +23,6 @@ WORKDIR /go/src/$DIR/traffic_monitor RUN bash -c "go get -u github.com/jstemmer/go-junit-report" RUN bash -c "go get -v" -CMD bash -c 'go test -v ./... ../lib/go-tc/... 2>&1 | go-junit-report --package-name=golang.test.tm --set-exit-code > /junit/golang.test.tm.xml && chmod 777 -R /junit && cat /junit/golang.test.tm.xml' -# +CMD bash -c 'go test -coverprofile=tmcoverage.out -v ./... ../lib/go-tc/... 2>&1 | go-junit-report --package-name=golang.test.tm --set-exit-code > /junit/golang.test.tm.xml && chmod 777 -R /junit && cat /junit/golang.test.tm.xml' +CMD bash -c 'go tool cover -func=tmcoverage.out' Review comment: Doing this disables the junit reporting, and actually the testing itself (including generation of `tmcoverage.out`). From [the Dockerfile Reference on `CMD`](https://docs.docker.com/engine/reference/builder/#cmd): > _"There can only be one CMD instruction in a Dockerfile. If you list more than one CMD then only the last CMD will take effect."_ So what you probably want is just to add this to the original `CMD` with an `&&` or something. If that's getting too complex to read/write/parse then perhaps we need a separate script to use as an `ENTRYPOINT` instead. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #3768: Codecov
ocket commented on a change in pull request #3768: Codecov URL: https://github.com/apache/trafficcontrol/pull/3768#discussion_r310637578 ## File path: traffic_ops/app/bin/tests/Dockerfile-golangtest ## @@ -20,6 +20,8 @@ ADD lib /go/src/$DIR/lib WORKDIR /go/src/$DIR/traffic_ops/traffic_ops_golang -CMD bash -c 'go get -v && go test -cover -v ./... ../../lib/go-tc/...' +CMD bash -c 'go get -v && go test -coverprofile=tocoverage.out -v ./... ../../lib/go-tc/...' +CMD bash -c 'go tool cover -func=tocoverage.out' Review comment: Same as above RE the number of `CMD`s in a Dockerfile This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #3768: Codecov
ocket commented on a change in pull request #3768: Codecov URL: https://github.com/apache/trafficcontrol/pull/3768#discussion_r310637394 ## File path: traffic_monitor/tests/Dockerfile-golangtest ## @@ -23,6 +23,6 @@ WORKDIR /go/src/$DIR/traffic_monitor RUN bash -c "go get -u github.com/jstemmer/go-junit-report" RUN bash -c "go get -v" -CMD bash -c 'go test -v ./... ../lib/go-tc/... 2>&1 | go-junit-report --package-name=golang.test.tm --set-exit-code > /junit/golang.test.tm.xml && chmod 777 -R /junit && cat /junit/golang.test.tm.xml' -# Review comment: I think you wanna leave this in. I don't use vim regularly enough to know for sure, but I think this empty comment line may be necessary for it to pick up the following syntax line. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
