Skip to content
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

Adds "skip" as a new testcaserun finished outcome #145

Merged
merged 1 commit into from
Mar 5, 2024

Conversation

dan-han-101
Copy link
Contributor

@dan-han-101 dan-han-101 commented Jun 26, 2023

Changes

Adds "skip" to the set of possible outcomes for a testcaserun.finished message. A final outcome of "skip" is common for many test frameworks, so adding it as a possible outcome will improve interoperability.

See further discussion in #140.

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

I clicked on all of the links in the submitter checklist, but some seem sort of stale or not very applicable.

dan-han-101 pushed a commit to dan-han-101/spec that referenced this pull request Jun 27, 2023
The 'skip' outcome is being added in
cdevents#145, so we'll update this document
assuming that this outcome will exist
@afrittoli
Copy link
Contributor

@dan-han-101 Emil will be back from PTO the second week of August - is it ok to wait until then?

@dan-han-101
Copy link
Contributor Author

Sure, it's no problem .
Thanks for checking in.

dan-han-101 pushed a commit to dan-han-101/spec that referenced this pull request Aug 1, 2023
The 'skip' outcome is being added in
cdevents#145, so we'll update this document
assuming that this outcome will exist

Signed-off-by: Daniel Han <[email protected]>
dan-han-101 added a commit to dan-han-101/spec that referenced this pull request Aug 1, 2023
The 'skip' outcome is being added in
cdevents#145, so we'll update this document
assuming that this outcome will exist

Signed-off-by: Daniel Han <[email protected]>
@e-backmark-ericsson
Copy link
Contributor

As mentioned in a comment on #146 I believe we need to gather a common view on what it means that a test cases is skipped vs canceled or aborted. It could be fine to add a skip outcome value to testCaseRun.finished, but I don't think it should be used to signal that a testcase execution was never even started.

@afrittoli
Copy link
Contributor

@dan-han-101 thanks for the updates! It looks like the examples need to be updated as well - see the outcome of the CI job.

@dan-han-101
Copy link
Contributor Author

@afrittoli - anything else I can do to help land this one? Thanks!

testing-events.md Show resolved Hide resolved
schemas/testcaserunfinished.json Outdated Show resolved Hide resolved
@e-backmark-ericsson
Copy link
Contributor

@afrittoli - anything else I can do to help land this one? Thanks!

Please make sure all unresolved conversations are sorted out and then resolved: https://github.com/cdevents/spec/pull/145/files#conversations-menu

testing-events.md Outdated Show resolved Hide resolved
testing-events.md Outdated Show resolved Hide resolved
@dan-han-101 dan-han-101 requested a review from a team as a code owner February 8, 2024 17:27
Copy link
Contributor

@e-backmark-ericsson e-backmark-ericsson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a couple of mentions of the environment property left in example and schema, which I believe should not be there for the skipped event. Apart from that this PR looks ok to me now.

examples/testcaserun_skipped.json Show resolved Hide resolved
schemas/testcaserunskipped.json Show resolved Hide resolved
Copy link
Contributor

@afrittoli afrittoli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The latest iteration looks good to me, thank you for your work on this @dan-han-101.
@e-backmark-ericsson @xibz wdyt?

Copy link
Contributor

@xibz xibz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dan-han-101 Thanks for the updates! This looks good :)

@dan-han-101
Copy link
Contributor Author

Hello. it looks like this one is now blocked because all commits need to be signed.

I'm not sure what's the best path forward here, as I'm not familiar with signing commits.

Would it be OK for me to squash all my commits, so that this PR only has a single commit, that I will make sure is signed?

Or is there an easy way to add sign off to old commits?

Or, could someone else merge this PR, by squashing all commits, and signing the finally commit ?

@afrittoli
Copy link
Contributor

Hello. it looks like this one is now blocked because all commits need to be signed.

I'm not sure what's the best path forward here, as I'm not familiar with signing commits.

Would it be OK for me to squash all my commits, so that this PR only has a single commit, that I will make sure is signed?

Or is there an easy way to add sign off to old commits?

Or, could someone else merge this PR, by squashing all commits, and signing the finally commit ?

Thanks @dan-han-101, sorry about the extra step 🙏
I would appreciate it if you could squash the commits and force-push. Alternatively, you can git rebase -i, mark all unsigned commits for edit and sign them with git commit --amend. Stack exchange suggests doing something like this, which seems reasonable:

git rebase --exec 'git commit --amend --no-edit -n -S' -i HEAD~<# of commits from HEAD you need to update>

so, probably in your case, it would be:

git rebase --exec 'git commit --amend --no-edit -n -S' -i HEAD~13

and then force-push.

@dan-han-101 dan-han-101 force-pushed the add.test.skip branch 2 times, most recently from 4bb0c32 to 8fa164a Compare March 1, 2024 20:18
Adds "skipped" as a new predicate for testcaserun events.
A final outcome of "skip" is common for many test frameworks,
so adding it as a possible outcome will improve interoperability.

See further discussion in cdevents#140.

Signed-off-by: Daniel Han <[email protected]>
@dan-han-101
Copy link
Contributor Author

@afrittoli - I think this is ready to land now. Please let me know if anything else is needed from me.

Copy link
Contributor

@afrittoli afrittoli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution and patience, this looks good now!

@afrittoli afrittoli merged commit 699dfa2 into cdevents:main Mar 5, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants