NO-JIRA: Clarify test names in OTE #1256
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@hongkailiu: This pull request explicitly references no jira issue. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hongkailiu 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 |
openshift-ci
bot
added
the
approved
|
/retest-required |
hack/update-test-metadata.sh
Outdated
| readonly METADATA_FILE=".openshift-tests-extension/openshift_payload_cluster-version-operator.json" | ||
| DELETE_EXISTING="${DELETE_EXISTING:-false}" | ||
|
|
||
| if [[ "${DELETE_EXISTING,,}" == "true" ]] && [[ -f "${METADATA_FILE}" ]] ; then |
There was a problem hiding this comment.
Why is this functionality needed?
There was a problem hiding this comment.
In my testing, cluster-version-operator-tests update is actually validating if the json file exists.
It creates only if the file does not exist.
There was a problem hiding this comment.
Removed.
A summary of slack discussion for the future of myself:
The expect dev workflow for renaming test:
- modify the name without going thro the renaming process: we have a hint (coming with this pull) reminding the process.
- dev did the process and rerun
make updateas hinted.
hack/update-test-metadata.sh
Outdated
|
|
||
| if [[ "${DELETE_EXISTING,,}" == "true" ]] && [[ -f "${METADATA_FILE}" ]] ; then | ||
| echo "Removing ${METADATA_FILE}" | ||
| rm .openshift-tests-extension/openshift_payload_cluster-version-operator.json |
There was a problem hiding this comment.
And this file name is dynamic, it was created by
, so it's better to find a dynamic way to delete it.@DavidHurta Please correct me if I am wrong.
There was a problem hiding this comment.
I believe this discussion was resolved in the relevant Slack thread. If not, please ping me.
This pull add a section to clarify test names in OTE. It modifies `hack/update-test-metadata.sh` to give a hint of what to do to fix the issue.
|
/cc |
| >&2 echo "run 'make update' to fix the metadata after going through the teest renaming process. | ||
| See https://github.com/openshift/cluster-version-operator/blob/main/cmd/cluster-version-operator-tests/README.md#test-names for details." |
There was a problem hiding this comment.
From the OTE enhancement update section:
If an incompatible change is introduced from the prior invocation of update (e.g. changing a test name without preserving the original), update will raise an error which the component owner must correct before committing their change in git.
Thus, it may not be just a renaming issue. Personally, I would make the message more general in nature; however, we can still reference the README file that can contain additional help.
|
|
||
| ### Deleting | ||
|
|
||
| If a test is renamed, claim it with [ext.IgnoreObsoleteTests](https://pkg.go.dev/github.com/openshift-eng/[email protected]/pkg/extension#Extension.IgnoreObsoleteTests) |
There was a problem hiding this comment.
| If a test is renamed, claim it with [ext.IgnoreObsoleteTests](https://pkg.go.dev/github.com/openshift-eng/[email protected]/pkg/extension#Extension.IgnoreObsoleteTests) | |
| If a test is deleted, claim it with [ext.IgnoreObsoleteTests](https://pkg.go.dev/github.com/openshift-eng/[email protected]/pkg/extension#Extension.IgnoreObsoleteTests) |
|
|
||
| # Update test metadata | ||
| eval "${BIN_PATH}/cluster-version-operator-tests" "update" | ||
| if ! "${BIN_PATH}/cluster-version-operator-tests" "update"; then |
There was a problem hiding this comment.
Comment not related to the code
We can break down the commit into two distinct commits as the current commit updates the README and also de facto updates the default logic of the make update target, and this way also updates the logic of the verify-update job, which are quite distinct areas. What do you think?
|
|
||
| ### Deleting | ||
|
|
||
| If a test is renamed, claim it with [ext.IgnoreObsoleteTests](https://pkg.go.dev/github.com/openshift-eng/[email protected]/pkg/extension#Extension.IgnoreObsoleteTests) |
There was a problem hiding this comment.
Clicking the link results in:
|
|
||
| ### Renaming | ||
|
|
||
| If a test is renamed, claim it with [specs.Walk](https://pkg.go.dev/github.com/openshift-eng/[email protected]/pkg/extension/extensiontests#ExtensionTestSpecs.Walk): |
There was a problem hiding this comment.
Clicking the link results in:
|
@hongkailiu: The following test failed, say
Full PR test history. Your PR dashboard. 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-sigs/prow repository. I understand the commands that are listed here. |
4 participants
This pull add a section to clarify test names in OTE.
It modifies
hack/update-test-metadata.shto give ahint of what to do to fix the issue.