tests: Handle some failures when using -race #739
Conversation
It's just nicer API for testing
3v1n0
force-pushed
the
some-goraces-handling
branch
from
342d4e1 to
3b403b0
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #739 +/- ##
==========================================
- Coverage 83.43% 83.07% -0.37%
==========================================
Files 83 96 +13
Lines 8689 9585 +896
Branches 74 74
==========================================
+ Hits 7250 7963 +713
- Misses 1111 1238 +127
- Partials 328 384 +56 ☔ View full report in Codecov by Sentry. |
| if !testutils.IsRace() || raceLog == "" { | ||
| return | ||
| } |
There was a problem hiding this comment.
That means we ignore go test exiting with a non-zero exit code if the race logfile is empty, right? Is that what we want?
There was a problem hiding this comment.
I think it also means we ignore any error if go test is not run with -race
There was a problem hiding this comment.
Ah we fail the test on https://github.com/3v1n0/authd/blob/3b403b006bcd14461f27163b0c7b65226b61e3f8/pam/integration-tests/vhs-helpers_test.go#L279-L279 if err is not nil
There was a problem hiding this comment.
However, we also fail on that line if we "ignored" the error in checkDataRace, right? What's the purpose of checkDataRace then?
There was a problem hiding this comment.
No, in case we detected the race, we're skiping the test... And Skip implies skipNow, thus no further checks will happen and we'll just go through the cleanups.
There was a problem hiding this comment.
Ah, I see. Do you mind making that a bit clearer in the code?
diff --git a/pam/integration-tests/vhs-helpers_test.go b/pam/integration-tests/vhs-helpers_test.go
index a9dd7c6b..ace4bada 100644
--- a/pam/integration-tests/vhs-helpers_test.go
+++ b/pam/integration-tests/vhs-helpers_test.go
@@ -336,14 +336,14 @@ func checkDataRace(t *testing.T, raceLog string) {
// https://github.com/charmbracelet/bubbletea/issues/909
// We can't do much here, as the workaround will likely affect the
// GUI behavior, but we ignore this since it's definitely not our bug.
- defer t.Skip("This is a very well known bubble tea bug (#909), ignoring it")
if testutils.IsVerbose() {
t.Logf("Ignored bubbletea race:\n%s", out)
- return
+ } else {
+ fmt.Fprintf(os.Stderr, "Ignored bubbletea race:\n%s", out)
}
- fmt.Fprintf(os.Stderr, "Ignored bubbletea race:\n%s", out)
- return
+ // This stops the execution of the test
+ t.Skip("This is a very well known bubble tea bug (#909), ignoring it")
}
t.Fatalf("Got a GO Race on vhs child:\n%s", out)There was a problem hiding this comment.
I've been taught here that using else is evil, thus the convolution :) but if you like it better...
There was a problem hiding this comment.
I've been taught here that using else is evil
As a rule of thumb I agree, but if it hurts clarity (like it in this case) I very much prefer clarity
There was a problem hiding this comment.
I've just dropped the logging, since it would happen still via skip, FWIW... And CI will include it.
|
|
||
| if testutils.IsRace() && strings.Contains(got, "WARNING: DATA RACE") && | ||
| strings.Contains(got, "bubbles/cursor.(*Model).BlinkCmd.func1") { | ||
| if strings.Contains(out, "bubbles/cursor.(*Model).BlinkCmd.func1") { |
There was a problem hiding this comment.
does each race logfile only contain one race? if not, we might ignore other detected races here if it happens in the same run as this well known race.
There was a problem hiding this comment.
No the log file contains all the races logs it detects AFAIK... Here i wanted to preserve the same behavior we had before.
Indeed, ideally we should parse it and still fail in case some other kind of race is detected, but I didn't want to go too far...
There was a problem hiding this comment.
Here i wanted to preserve the same behavior we had before.
Fair enough, but then let's add a // TODO comment
3v1n0
force-pushed
the
some-goraces-handling
branch
2 times, most recently
from
74f8df0 to
f6d13c8
Compare
Errno may be accessed by other code during the go execution, breaking our assumptions. So let's test that our code is race-free by using a test variable instead.
…vhs output Since the introduction of the VHS wait feature we may get the data race output as VHS error, so let's handle it earlier if vhs process fails. We still keep the check in the golden file, as we may still get a race output after a successful Wait. Closes: ubuntu#703
We were relying on the process output to check on data races, while Go allows us to write the race content
3v1n0
force-pushed
the
some-goraces-handling
branch
from
f6d13c8 to
6c1bf10
Compare
See commits for details.
Close: #703
UDENG-5710