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
e2e: add cargo-based e2e test and run it after the instance is stood up #202
e2e: add cargo-based e2e test and run it after the instance is stood up #202
Conversation
7dbb51f
to
627bda2
Compare
627bda2
to
e20007b
Compare
hack/e2e.sh
Outdated
curl --header 'Accept:application/json' "${GRAPH_URL}" | ||
# Run e2e tests | ||
cd ../../graph-builder | ||
cargo test --features test-e2e e2e --no-default-features -- --nocapture |
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 would install test dependencies during the test run, which is suboptimal. I'll rework this in /release repo to ensure a test image is built first
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.
I'm not sure what you mean by "install test dependencies", please explain.
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 test starts with a container built from Dockerfile.builder
, so it has rustc and cargo installed, but doesn't have test prebuilt. So when cargo test
is run here it would download crates and compile test - log
I'll update this to have test built along with other artifacts in follow up PRs
f5288a4
to
7d2cb9e
Compare
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.
Just a few nitpicks in the comments.
Extra note:
I'm not a fan of our using and with this PR duplicating the expected hashes from the manually maintained test repositories, see e.g. here. A first step is to have a common definition of these images and their expected hashes somewhere in shared module. We can postpone this to a future PR but I did want to note this down somewhere.
This enables Cincinnati to fit on a standard openshift cluster
71a2b3f
to
ec8b65d
Compare
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.
/lgtm
Thanks!
I'm not sure if you wanted to do something about #202 (comment) in this PR or postpone all of the things you mentioned, so:
/hold
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: steveeJ, vrutkovs The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This needs to happen in two repos - first proper images built in openshift/release then these images would be used in cincinnati's e2e. I'll keep it on hold until e2e passes just to be sure |
e2e passed \o/ /hold cancel |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
3 similar comments
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/refresh |
@vrutkovs: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
/override ci/prow/rustfmt-1.38.0 Outdated test |
@vrutkovs: Overrode contexts on behalf of vrutkovs: ci/prow/rustfmt-1.38.0 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Add several tests which use
reqwest
to ensure returned test data graph is valid. These tests won't run duringcargo test
, instead they'd be a part of e2e suitePart of OTA-103