[wip] skip tests based on central registry#81293
[wip] skip tests based on central registry#81293tbg wants to merge 1 commit intocockroachdb:masterfrom
Conversation
This has a big problem: we need each subtest to invoke leaktest. So this isn't as useful as I thought though it's still a lot better than skipping manually. But with this as is there's a good chance someone will use the registry to skip a test but it doesn't actually skip, and that surely sucks Release note: None
|
This change is |
With a bit of hack we could pass our own predicate used for matching all tests, including subtests, The actual predicate could just do exact matches against the registry. The same predicate is propagated into subtests [1]. [1] https://github.com/golang/go/blob/master/src/testing/testing.go#L1831 |
|
Good sleuthing! Let's investigate this approach. I also wanted to mention yet another approach - we could rally around TeamCity's mute feature. Pro
Con
|
Indeed, I considered that option too but ruled against it for the same reasons; plus, we don't want tight coupling with TeamCity because some day we just might end up using something else. I think we can make this work via
It invokes I will do a more exhaustive test tomorrow. Note that we can auto-generate [1] https://github.com/golang/go/blob/master/src/testing/testing.go#L1603 |
|
Nice! This should really do it. We could even call Have you thought where to source the list of skipped tests from? Avoiding the CI round-trip is helpful, and yet this stuff needs to be maintained by branch so it's difficult to do it with an env var in TC. Besides, we ultimately want an API endpoint to interact with the skipped tests. And, also, we do want the tests to be skipped in local development! And probably we also want to avoid building too much of "our own". Probably at first env vars are still the way to go to get off the ground ASAP, with one per branch. For example, on the Cockroach project, we'd have an env var
We would probably pivot on the format a few times (as we find a need to maybe only skip tests for purpose of CI, but not for purpose of nightlies, etc, and maybe we'd get in the habit of putting an expiration date for the "mute" on each skip to prevent rot, who knows). The next question is what processes we need around this new way to skip tests, to make sure that it is discoverable and that folks across the org have a good view of which tests are skipped. Maybe rather than the env var, we should right away put the skiplist into something like a
I'm not sure I'm too worried about that. If TC gets the job done, and we are principled about interacting with it using clean APIs, it wouldn't be too hard to switch it out. But I think it will be hard to use anyway. |
What about a github repo? We already have the tooling to pull/push. We could either inline the branch into the file format or use an actual branch with the same name; I'd prefer the latter. This way we have the full revision history of a skiplist per branch.
Right, the coupling with TC is not a deal breaker. But the alternative seems more flexible and portable. |
Yeah, that seems nice. Agree re: using branch names for the skiplists. We will need to create release branches on that repo as well then, so it does create a little bit of extra work. We could entertain using the main repo for this instead (i.e. go back to putting this stuff in the actual release branches, next to code) and suitably speed up CI for them (bypass bors and don't run general tests, just test the skip file in ~seconds). One interface could be |
When a test fails, --flaky_test_attempts=3 will cause up to two retry attempts by executing the corresponding test binary (shard). If a test passes on either attempt, it is reported 'FLAKY' in the test result summary. In addition, each attempt yields an (XML) log under 'test_attempts' subdirectory. The attempts are now copied into the artifactsDir. Note, while the test result summary may say the test is 'FLAKY', it's reported as 'PASS' as per main test.xml. Thus, this change doesn't change the status of CI, semantically; i.e., a test succeeding after up to two retry attempts is assumed to have _passed_ whereas three consecutive failures result in a failed test. However, the two additional attempts per test binary (shard) could slow down the build. Hence, initially the plan is to enable only on release branches and not staging (i.e., bors). This change supports [1]. In a follow-up PR, a test reporter could be augmented to parse attempt logs thereby obtaining a set of FLAKY tests which should be skipped on subsequent builds. [1] cockroachdb#81293 Release justification: CI improvement Release note: None
When a test fails, --flaky_test_attempts=3 will cause up to two retry attempts by executing the corresponding test binary (shard). If a test passes on either attempt, it is reported 'FLAKY' in the test result summary. In addition, each attempt yields an (XML) log under 'test_attempts' subdirectory. The attempts are now copied into the artifactsDir. Note, while the test result summary may say the test is 'FLAKY', it's reported as 'PASS' as per main test.xml. Thus, a 'FLAKY' test will not fail the CI; i.e., a test succeeding after up to two retries is assumed to have _passed_ whereas three consecutive failures result in a failed test. Consequently, CI now has a higher probability of passing than before this change. However, the two additional attempts per test binary (shard) could slow down the build. Hence, initially the plan is to enable only on release branches and not staging (i.e., bors). This change supports [1]. In a follow-up PR, a test reporter could be augmented to parse attempt logs thereby obtaining a set of FLAKY tests which should be skipped on subsequent builds. [1] cockroachdb#81293 Release justification: CI improvement Release note: None
When a test fails, --flaky_test_attempts=3 will cause up to two retry attempts by executing the corresponding test binary (shard). If a test passes on either attempt, it is reported 'FLAKY' in the test result summary. In addition, each attempt yields an (XML) log under 'test_attempts' subdirectory. The attempts are now copied into the artifactsDir. Note, while the test result summary may say the test is 'FLAKY', it's reported as 'PASS' as per main test.xml. Thus, a 'FLAKY' test will not fail the CI; i.e., a test succeeding after up to two retries is assumed to have _passed_ whereas three consecutive failures result in a failed test. Consequently, CI now has a higher probability of passing than before this change. However, the two additional attempts per test binary (shard) could slow down the build. Hence, initially the plan is to enable only on release branches and not staging (i.e., bors). This change supports [1]. In a follow-up PR, a test reporter could be augmented to parse attempt logs thereby obtaining a set of FLAKY tests which should be skipped on subsequent builds. [1] cockroachdb#81293 Release justification: CI improvement Release note: None
86891: bazci: enable --flaky_test_attempts=3 for Unit tests on release branches r=rickystewart,healthy-pod,renatolabs a=srosenberg When a test fails, --flaky_test_attempts=3 will cause up to two retry attempts by executing the corresponding test binary (shard). If a test passes on either attempt, it is reported 'FLAKY' in the test result summary. In addition, each attempt yields an (XML) log under 'test_attempts' subdirectory. The attempts are now copied into the artifactsDir. Note, while the test result summary may say the test is 'FLAKY', it's reported as 'PASS' as per main test.xml. Thus, a 'FLAKY' test will not fail the CI; i.e., a test succeeding after up to two retries is assumed to have _passed_ whereas three consecutive failures result in a failed test. Consequently, CI now has a higher probability of passing than before this change. However, the two additional attempts per test binary (shard) could slow down the build. Hence, initially the plan is to enable only on release branches and not staging (i.e., bors). This change supports [1]. In a follow-up PR, a test reporter could be augmented to parse attempt logs thereby obtaining a set of FLAKY tests which should be skipped on subsequent builds. [1] #81293 Release justification: CI improvement Release note: None Epic: None 92955: opt: inline UDFs as subqueries r=mgartner a=mgartner UDFs are now inlined as subqueries by a normalization rule when possible, speeding up their evaluation. Epic: CRDB-20370 Release note (performance improvement): Some types of user-defined functions are now inlined in query plans as relation expressions, which speeds up their evaluation. UDFs must be non-volatile and have a single statement in the function body to be inlined. 94660: colserde: perform memory accounting for scratch slices in the converter r=yuzefovich a=yuzefovich This commit introduces the memory accounting for values and offsets scratch slices that are being reused across `BatchToArrow` calls since recently. This required a lot of plumbing to propagate the memory account from all places. The converter is careful to only grow and shrink the account by its own usage, so the same memory account can be reused across multiple converters (as long as there is no concurrency going on, and we only can have concurrency for hash router outputs, so there we give each output a separate account). An additional minor change is that now in `diskQueue.Enqueue` we properly `Close` the `FileSerializer` before `nil`-ing it out. This isn't a problem per se since it is the caller's responsibility to close the account used by the serializer, but it's nice to properly release the accounted for bytes. Epic: None Release note: None 94702: workload: fix non-determinism in TPC-H data generation r=rytaft a=rytaft This commit fixes the non-determinism in the TPC-H data generation by using a local slice for the random permutation of indexes into `randPartNames`. It also fixes the permutation algorithm to use a modified Fisher–Yates shuffle. Fixes #93958 Release note: None 94713: sql: add user-facing error for invalid job type in job_payload_type builtin r=jayshrivastava a=jayshrivastava ### sql: add user-facing error for invalid job type in job_payload_type builtin Previously, the `job_payload_type` builtin would return an internal error if the payload could be unmarshalled, but the type could not be determined. This changes ensures the builtin returns an appropriate user-facing error in this case. Epic: none Fixes: #94680 Release note: None 94723: kvserver: check lease rebalance target exists in cached storelist r=erikgrinaker a=kvoli From #91593 it was possible for a new lease target to have its store descriptor dereferenced despite not existing in a list of candidate stores earlier. This patch resolves the dereference. fixes: #94688 Release note: None Co-authored-by: Stan Rosenberg <stan@cockroachlabs.com> Co-authored-by: Marcus Gartner <marcus@cockroachlabs.com> Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com> Co-authored-by: Rebecca Taft <becca@cockroachlabs.com> Co-authored-by: Jayant Shrivastava <jayants@cockroachlabs.com> Co-authored-by: Austen McClernon <austen@cockroachlabs.com>
This has a big problem: we need each subtest to invoke leaktest.
So this isn't as useful as I thought though it's still a lot better
than skipping manually. But with this as is there's a good chance
someone will use the registry to skip a test but it doesn't actually
skip, and that surely sucks
Release note: None