fix: flaky test #2961
fix: flaky test #2961
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2961 +/- ##
==========================================
+ Coverage 75.89% 76.25% +0.35%
==========================================
Files 311 309 -2
Lines 19185 19032 -153
==========================================
- Hits 14560 14512 -48
+ Misses 3486 3396 -90
+ Partials 1139 1124 -15
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
fix test? Revert "hack: try to fix test" This reverts commit 213b8fa. add some explanations
CaptainStandby
force-pushed
the
hf-fix-build
branch
from
004ccc0 to
2e7aa53
Compare
CaptainStandby
force-pushed
the
hf-fix-build
branch
from
7996ad3 to
1397fcc
Compare
|
|
||
| // There are a bunch of log messages beeing logged. We are looking for a specific one. | ||
| timeout := time.After(time.Millisecond * 500) | ||
| outer: |
There was a problem hiding this comment.
If possible try to avoid labels for better readability :)
|
|
||
| select { | ||
| case <-ctx.Done(): | ||
| panic("the test could not complete as the context timed out before the file watcher updated") |
| for _, v := range hook.AllEntries() { | ||
| s, err := v.String() | ||
| if err != nil { | ||
| t.Errorf("Unexpected Error: %s", err.Error()) |
There was a problem hiding this comment.
Please use require.NoError or assert.NoError
|
@CaptainStandby can you please address the comments in a follow-up PR? Thank you! |
There was a problem hiding this comment.
I also investigated this already. The underlying issue is that marshalAndWrite causes two notifications (one for .Truncate and one for .Write).
The file content is then read after the first event. Depending on the speed of the system/disk this can then flake, returning invalid content (i.e. empty file), or the expected content. Ideally we would do an atomic file write in the test instead, but this is also OK.
|
Yeah, we noticed this too and the first take on fixing this, was doing atomic file write. But we decided to go with this approach, because we wanted to retain the "flakyness" of the write to test that the system could correctly deal with that situation. |
|
Yes, the non-atomic write is almost certainly the problem. Like @CaptainStandby said, we purposefully kept that test behavior because non-atomic writes could easily happen in real-world use, too. Instead of changing the test to use an atomic move, we more broadly inspect what's logged. A similar flakyness could happen if the logger internally re-ordered log lines. |
This fixes a problematic unit test, that monitors log messages and expected a specific message at the end of the list. This has been corrected by searching for the expected message in the list.
Related issue(s)
Checklist
introduces a new feature.
contributing code guidelines.
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got the approval (please contact
security@ory.sh) from the maintainers to push
the changes.
works.
Further Comments