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

kola/harness.go: add info to recently expired snooze #3515

Closed
wants to merge 1 commit into from

Conversation

c4rt0
Copy link
Member

@c4rt0 c4rt0 commented Jun 21, 2023

Fix: #3462

@openshift-ci
Copy link

openshift-ci bot commented Jun 21, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@c4rt0 c4rt0 force-pushed the log_more_info branch 2 times, most recently from fa1a2e0 to d06ac67 Compare June 21, 2023 17:22
@c4rt0 c4rt0 requested a review from dustymabe June 22, 2023 20:11
@c4rt0 c4rt0 marked this pull request as ready for review June 22, 2023 20:13
@cverna
Copy link
Member

cverna commented Jun 23, 2023

From a quick look, you probably want to squash your commits :-)

@c4rt0
Copy link
Member Author

c4rt0 commented Jun 23, 2023

From a quick look, you probably want to squash your commits :-)

Now that was a quick response, thanks :)

Copy link
Member

@travier travier left a comment

Choose a reason for hiding this comment

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

I added a super important review comment ™️

mantle/kola/harness.go Outdated Show resolved Hide resolved
@c4rt0
Copy link
Member Author

c4rt0 commented Jun 23, 2023

/retest

@dustymabe
Copy link
Member

This currently doesn't tell me much information about which test was unsnoozed recently:

kola -p qemu run fcos.internet --output-dir tmp/kola
⏰ Snooze expired "Jun 20 2023":
⚠️  Skipping kola test pattern "podman.workflow":
  👉 https://github.com/coreos/coreos-assembler/pull/1478
=== RUN   fcos.internet
=== RUN   fcos.internet/PodmanEcho
=== RUN   fcos.internet/PodmanWgetHead
--- PASS: fcos.internet (27.23s)
    --- PASS: fcos.internet/PodmanEcho (1.60s)
    --- PASS: fcos.internet/PodmanWgetHead (0.29s)
PASS, output in tmp/kola
+ rc=0
+ set +x

@dustymabe
Copy link
Member

I think this information would be most useful if it was presented to the user along with the failure message.. i.e. The user is more likely to skip over the "header" at the top and just look at the failure message itself and we'll end up in the same situation we are in today where someone goes down the road of investigating a test failure when the problem is already well known.

If we could get something like what was described in #3462 (comment) that would be more helpful (though I admit this would be harder to achieve):

[2023-05-09T02:57:34.826Z] --- FAIL: ext.config.shared.networking.nmstate.state (27.96s) snooze expired 2023-05-08

I'd be happy to modify that to include an emoji :)

What you have in this PR already is useful too, though. So we could maybe print the information at the top like you are doing now AND include it along with a failure.

WDYT?

@c4rt0
Copy link
Member Author

c4rt0 commented Jun 23, 2023

This currently doesn't tell me much information about which test was unsnoozed recently:

kola -p qemu run fcos.internet --output-dir tmp/kola
⏰ Snooze expired "Jun 20 2023":
⚠️  Skipping kola test pattern "podman.workflow":
  👉 https://github.com/coreos/coreos-assembler/pull/1478
=== RUN   fcos.internet
=== RUN   fcos.internet/PodmanEcho
=== RUN   fcos.internet/PodmanWgetHead
--- PASS: fcos.internet (27.23s)
    --- PASS: fcos.internet/PodmanEcho (1.60s)
    --- PASS: fcos.internet/PodmanWgetHead (0.29s)
PASS, output in tmp/kola
+ rc=0
+ set +x

Agreed.

I think this information would be most useful if it was presented to the user along with the failure message.. i.e. The user is more likely to skip over the "header" at the top and just look at the failure message itself and we'll end up in the same situation we are in today where someone goes down the road of investigating a test failure when the problem is already well known.

If we could get something like what was described in #3462 (comment) that would be more helpful (though I admit this would be harder to achieve):

[2023-05-09T02:57:34.826Z] --- FAIL: ext.config.shared.networking.nmstate.state (27.96s) snooze expired 2023-05-08

I'd be happy to modify that to include an emoji :)

What you have in this PR already is useful too, though. So we could maybe print the information at the top like you are doing now AND include it along with a failure.

WDYT?

I will try now to include it along with the failure, as it was originally stated in the linked issue.

mantle/cmd/kola/testiso.go Outdated Show resolved Hide resolved
mantle/kola/harness.go Outdated Show resolved Hide resolved
// CheckLastSnooze is verifying if the last snooze recently expired
func CheckLastSnooze(snoozeDate string) {
if snoozeDate != "" {
snoozeDate, err := time.Parse(kola.SnoozeFormat, snoozeDate)
Copy link
Member

Choose a reason for hiding this comment

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

Should that be snoozeFormat due that the revert change you did?

@c4rt0
Copy link
Member Author

c4rt0 commented Aug 16, 2023

/do-not-merge

@c4rt0
Copy link
Member Author

c4rt0 commented Aug 21, 2023

/ok-to-test

@c4rt0
Copy link
Member Author

c4rt0 commented Aug 21, 2023

/retest

@c4rt0
Copy link
Member Author

c4rt0 commented Aug 21, 2023

Just to summarize the work on this issue I would like to extend my gratitude to everyone for their patience. I was stuck in a loophole trying to re-work what was already done some months ago. Since I had no previous exposure to Go I was taking time in finding a way around it. First I was focusing on the testiso.go where I spotted an opportunity to modify 'FAIL' result prompt. Without an understanding of the correct usage of functions/methods in Go I was ignoring the bool return type for a while in the printResult function. After that lesson was learned I attempted to extract data from the DenyListObj not remembering that first yaml object must be parsed in order to extract the data. As a result, I am now focusing on the ParseDenyListYaml function which already does everything I needed in order to address considered here issue.

@c4rt0
Copy link
Member Author

c4rt0 commented Sep 19, 2023

Due to recent developments, this is no longer required.

@c4rt0 c4rt0 closed this Sep 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

kola: log more info when test fails that recently had snooze expire
6 participants