feat: Generating e2e test coverage #669
Conversation
chandankumar4
force-pushed
the
e2e-coverage
branch
4 times, most recently
from
1f79726 to
ed031d5
Compare
chandankumar4
changed the title
Signed-off-by: chandankumar4 <[email protected]>
chandankumar4
force-pushed
the
e2e-coverage
branch
13 times, most recently
from
df8b26c to
5bbbdd0
Compare
Signed-off-by: chandankumar4 <[email protected]>
chandankumar4
force-pushed
the
e2e-coverage
branch
from
5bbbdd0 to
31b07f5
Compare
| # especially steps that involve installing packages using a package manager. | ||
| ENV GOCACHE=/root/.cache/go-build | ||
| # Build | ||
| # the GOARCH has not a default value to allow the binary be built according to the host where the command |
There was a problem hiding this comment.
I know this was copied from the other Dockerfile, but do you mind fixing the phrasing to "the GOARCH doesn't have a default value"? (in both Dockerfiles)
| # by leaving it empty we can ensure that the container and binary shipped on it will have the same platform. | ||
| RUN --mount=type=cache,target="/root/.cache/go-build" CGO_ENABLED=0 GOOS=${TARGETOS:-linux} GOARCH=${TARGETARCH} go build -cover -coverpkg=./... -o manager cmd/main.go | ||
|
|
||
| ## Use alpine as minimal base image to package the manager binary |
There was a problem hiding this comment.
this comment isn't right for this Dockerfile
| ## Use alpine as minimal base image to package the manager binary | ||
| FROM golang:1.23 | ||
| WORKDIR / | ||
| COPY --from=builder /workspace/manager . |
There was a problem hiding this comment.
that's interesting that it works without copying the code itself in. I guess the binary alone must be aware of all of the code lines (or whatever it needs to generate the coverage report)
There was a problem hiding this comment.
btw, I know this PR is still a WIP, but when you get to it, make sure to add some comments in here about what you're doing
| volumes: | ||
| - name: config-volume | ||
| configMap: | ||
| name: numaplane-controller-config |
There was a problem hiding this comment.
it will be easy to forget to update this file when our Deployment changes - can we change it to a patch?
| ./hack/numaflow-controller-def-generator/numaflow-controller-def-generator.sh | ||
| $(KUBECTL) apply -f $(TEST_MANIFEST_DIR_DEFAULT)/numaplane-ns.yaml | ||
| $(KUBECTL) kustomize $(TEST_MANIFEST_DIR) | sed '[email protected]/numaproj/@$(IMAGE_NAMESPACE)/@' | sed 's/$(IMG):$(BASE_VERSION)/$(IMG):$(VERSION)/' | $(KUBECTL) apply -f - | ||
| cat tests/e2e/coverage/controller-manager.yaml | sed 's/$(IMG):$(BASE_VERSION)/$(IMG):$(VERSION)/' | $(KUBECTL) apply -f - |
There was a problem hiding this comment.
The only difference between this and make start is the last line, right? How about we just control that with an environment variable? (Also, this will need to change based on my other comment)
| $(CONTAINER_TOOL) build -t ${IMAGE_FULL_PATH} -f tests/e2e/coverage/Dockerfile . | ||
| ifdef IMAGE_IMPORT_CMD | ||
| $(IMAGE_IMPORT_CMD) ${IMAGE_FULL_PATH} | ||
| endif |
There was a problem hiding this comment.
may be able to consolidate this and make image into one by using an environment variable for the Dockerfile
| app.kubernetes.io/component: controller-manager | ||
| spec: | ||
| containers: | ||
| - image: quay.io/numaproj/numaplane-controller:latest |
There was a problem hiding this comment.
I wonder if we want a distinct image name or image tag for this one, to avoid confusion
| POD_NAME=$(kubectl get pods -n numaplane-system -l app.kubernetes.io/name=controller-manager -o jsonpath="{.items[0].metadata.name}") | ||
| kubectl exec -n numaplane-system $POD_NAME -- /bin/bash -c 'pkill -f "/manager --health-probe-bind-address=:8081 --metrics-bind-address=:8080 --leader-elect"' | ||
| sleep 5s | ||
| kubectl exec -n numaplane-system $POD_NAME -- /bin/bash -c 'pkill -f "/manager --health-probe-bind-address=:8081 --metrics-bind-address=:8080 --leader-elect"' |
There was a problem hiding this comment.
why did you have to kill it twice?
| run: | | ||
| mkdir -p tests/e2e/output/resources/coverage/ | ||
| POD_NAME=$(kubectl get pods -n numaplane-system -l app.kubernetes.io/name=controller-manager -o jsonpath="{.items[0].metadata.name}") | ||
| kubectl cp $POD_NAME:/coverage -n numaplane-system tests/e2e/output/resources/coverage/ |
There was a problem hiding this comment.
this is great. So, we can have one or more steps to:
- aggregate all of the e2e coverage, post that as an artifact
- post the unit test code coverage as an artifact
- aggregate e2e and unit test code coverage together and post that as an artifact
Ideal thing is to be able to have html output for these artifacts that we could view in a browser
There was a problem hiding this comment.
actually, you can see if we need artifacts at all, or if posting to CodeCov gives us everything we need, now that I think about it
Fixes #620
Modifications
Verification
Backward incompatibilities