Disable failing when TESTS not found in TEST_SKIPS#22594
Disable failing when TESTS not found in TEST_SKIPS#22594ardaguclu wants to merge 1 commit intoopenshift:masterfrom
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ardaguclu The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
/assign @andfasano As it is suspected, it still fails, we need to catch error and gracefully return to allow process continues. Could you PTAL?, thanks. |
|
/title Disable failing when TESTS not found in TEST_SKIPS |
ardaguclu
changed the title
|
@ardaguclu: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
| echo "${TESTS}" | grep -v "${TEST_SKIPS}" >/tmp/tests && | ||
| echo "Skipping tests:" && | ||
| echo "${TESTS}" | grep "${TEST_SKIPS}" || { exit_code=$?; echo 'Error: no tests were found matching the TEST_SKIPS regex:'; echo "$TEST_SKIPS"; return $exit_code; } && | ||
| echo "${TESTS}" | grep "${TEST_SKIPS}" || { echo 'Error: no tests were found matching the TEST_SKIPS regex:'; echo "$TEST_SKIPS"; } && |
There was a problem hiding this comment.
I think all of the &&s are unnecessary now, since this line always succeeds. It'd be significantly easier to read without them.
Also it's a bit weird to print 'Error' for something that isn't actually an error.
There was a problem hiding this comment.
I totally agree all of your comments @zaneb. Our intention was to be similar with other workflows in here
But I think, we can remove && and change the message from Error to something Info.
What do you think @andfasano ?
There was a problem hiding this comment.
Agree, but given that our long-term goal is to adopt directly the openshift-e2e-test step (and abandon baremetalds-e2-test one - also PR #19987 goes in that direction), any discrepancy - not baremetal specific - introduced now will make more painful to merge our changes into the openshift-e2e-test when we'll be ready to do it.
So in this PR I'll stick as much as possible closer to the openshift-e2e-test step for whatever is not baremetal specific - and eventually address later any improvement directly in that step once we'll get it.
|
As stated in here #22755 (comment). Workflows can use their own Thus, I'm also closing this PR. /close |
|
@ardaguclu: Closed this PR. DetailsIn 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. |
3 participants
Previously we are inspired by
release/ci-operator/step-registry/openshift/e2e/test/openshift-e2e-test-commands.sh
Line 239 in 30d7dab
However, when TESTS are not in TEST_SKIPS section, grep still fails and we don't catch it. For example, https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/openshift_release/22490/rehearse-22490-pull-ci-openshift-cluster-baremetal-operator-release-4.10-e2e-metal-ipi-serial-compact/1446421249919029248/artifacts/e2e-metal-ipi-serial-compact/baremetalds-e2e-test/build-log.txt
openshift-e2e-test-commands.shhas the same problem, but it works due to TEST_SKIPS section is empty.release/ci-operator/step-registry/openshift/e2e/test/openshift-e2e-test-ref.yaml
Line 40 in 30d7dab
This PR catches grep not found failure and gracefully returns.