bazel: tweak logic for staging test artifacts in bazci #63767
Conversation
|
This change is |
|
Ah, I think this isn't behaving exactly how I wanted to, recalling for now :) |
|
days later, i realized it's because go switch statements don't work like they do in C 🙃 |
There was a problem hiding this comment.
In overall it look good to me. There are a couple of small changes.
Reviewed 1 of 1 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @rickystewart)
pkg/cmd/bazci/watch.go, line 73 at r1 (raw file):
fileToStaged map[string]bool
I think you can use empty structs here - they are memory friendly than bool. https://dave.cheney.net/2014/03/25/the-empty-struct is a good read about those.
pkg/cmd/bazci/watch.go, line 253 at r1 (raw file):
} var _ io.WriteCloser = (*cancelableWriter)(nil)
Did you add this to make sure we implement the interface? Can you add a short comment please.
There was a problem hiding this comment.
Reviewable status:
pkg/cmd/bazci/watch.go, line 253 at r1 (raw file):
Previously, rail (Rail Aliiev) wrote…
Did you add this to make sure we implement the interface? Can you add a short comment please.
Done.
Because Bazel aggressively caches build/test artifacts, if we're not careful, bazci can copy OLD artifacts from previous build/test runs into the artifacts directory. This is particularly an issue because TeamCity watches that directory for `test.xml` reports, and if an old `test.xml` reports a test failure, TeamCity will notice that and report the failure in the UI -- and importantly, even if we replace that `test.xml` with a completely different one that reports that the test succeeed, TC will not amend what's displayed in the UI accordingly. So this can manifest as reported test failures from unrelated PR's showing up in TC in an apparently unpredictable (though uncommmon) manner. We fix this by making bazci a little smarter about when we choose to stage artifacts: 1. The first time the watcher loops over all the test artifacts, never stage anything (the artifacts are probably cached -- not enough time has passed for any legitimate artifacts to appear, probably). 2. Only stage artifacts incrementally if their stats have changed since the initial round of caching. 3. During the final loop, stage ALL artifacts (if they haven't been staged yet), just to make sure we don't miss anything. Also add lots of comments to make these design decisions and their motivations a little clearer. Resolves cockroachdb#63740. Release note: None
There was a problem hiding this comment.
Reviewable status:
|
bors r=rail |
|
Build failed (retrying...): |
|
Build succeeded: |
Because Bazel aggressively caches build/test artifacts, if we're not
careful, bazci can copy OLD artifacts from previous build/test runs into
the artifacts directory. This is particularly an issue because TeamCity
watches that directory for
test.xmlreports, and if an oldtest.xmlreports a test failure, TeamCity will notice that and report the failure
in the UI -- and importantly, even if we replace that
test.xmlwith acompletely different one that reports that the test succeeed, TC will
not amend what's displayed in the UI accordingly. So this can manifest
as reported test failures from unrelated PR's showing up in TC in an
apparently unpredictable (though uncommmon) manner.
We fix this by making bazci a little smarter about when we choose to
stage artifacts:
stage anything (the artifacts are probably cached -- not enough time
has passed for any legitimate artifacts to appear, probably).
the initial round of caching.
staged yet), just to make sure we don't miss anything.
Also add lots of comments to make these design decisions and their
motivations a little clearer.
Resolves #63740.
Release note: None