cockroachdb / cockroach

CockroachDB — the cloud native, distributed SQL database designed for high availability, effortless scale, and control over data placement.
https://www.cockroachlabs.com
Other
30.07k stars 3.8k forks source link

roachtest: acceptance subtests interact poorly with teamcity-post-failures.py #30548

Closed tbg closed 3 years ago

tbg commented 6 years ago

We group all acceptance roachtests into an umbrella test. This means that when teamcity-post-failures.py posts an issue, it will never reference the test that actually failed in the title but instead create useless barrages of issues like these:

image

Arguably the python issue poster is pretty bad anyway, but this is something that the roachtests get wrong. We've taught our issue poster to not create multiple issues for subtests, but in roachtest/acceptance we do want that.

Do we want to introduce an extra layer that determines on a per-entity basis whether subtests are to be spelled out? We could do so via an env var in the build. Any other ideas?

cc @petermattis

tbg commented 6 years ago

Perhaps the right thing to do here is to not group subtests out any more, and to instead change the tests that create spurious issues instead. That might be more manageable than putting implicit assumptions that are kind of easy to break everywhere.

petermattis commented 6 years ago

Couldn't we have roachtest post these issues? I think that is just a bit of env var setup. I know you were wondering why roachtest posts issues directly elsewhere, but it seems to be easily solvable in roachtest vs more awkwardly solvable externally.

tbg commented 6 years ago

One "problem" here is that by saying that roachtest will post its own failures, we have to teach the other scripts to ignore failures from roachtest. That is doable as well, but before deciding one way or another I want to double check which solution results in the fewest headaches. Consider the argument:

  1. we've basically rewritten a good amount of the go test framework outside of go already and are unlikely to back out of that, and so
  2. we have and will always have output in the Go test format, and so
  3. we should be using our existing infra that eats Go test output and not add yet another set of code to maintain

On the other hand, roachtest failures are different from other failures (they don't want a stressrace invocation, not sure if there are other differences)

Your argument for posting failures as they occur and not much later is reasonable, though I personally prefer it the other way (less interruptions).

petermattis commented 6 years ago

we should be using our existing infra that eats Go test output and not add yet another set of code to maintain

On the bright side, roachtest does share the issue posting code with with github-post. I thought @benesch wanted to get rid of this python code at some point. Perhaps I'm misremembering.

Your argument for posting failures as they occur and not much later is reasonable, though I personally prefer it the other way (less interruptions).

Btw, this is less of a concern now that we've broken the most egregiously long running roachtests into smaller pieces (i.e. jepsen).

benesch commented 6 years ago

On the bright side, roachtest does share the issue posting code with with github-post. I thought @benesch wanted to get rid of this python code at some point. Perhaps I'm misremembering.

You're not misremembering. I think @tschottdorf's plan is to rewrite that Python script as a simple wrapper around the github-post package.

petermattis commented 6 years ago

@andreimatei Tossing your way because I believe your plan is to get rid of subtests.

andreimatei commented 6 years ago

I am getting rid of subtests indeed, but that won't, in itself, affect the output. As long as a test's name is "acceptance/foo" (be it a roachtest subtest or not), I imagine that this python script will do what it does now. Tossing this back to... Tobi as... the owner of the python script :P For acceptance in particular, we could stop naming tests "acceptance/foo", as I don't think the prefix adds anything. But in general we do have tests named "tpcc/foo" and "schemachange/foo". I don't know how we want issues grouped for these.

Btw, as the discussion below showed, there's a difference between how issues are filed for the nightly roachtest run (which doesn't use this python script) versus the roachtests that run on merge. https://groups.google.com/a/cockroachlabs.com/d/msgid/eng/CAPqkKgmFp4wRU%2BVxFwoFDngF3b_84s67WP7XfFE14ENEYB%3DLow%40mail.gmail.com.

github-actions[bot] commented 3 years ago

We have marked this issue as stale because it has been inactive for 18 months. If this issue is still relevant, removing the stale label or adding a comment will keep it active. Otherwise, we'll close it in 5 days to keep the issue queue tidy. Thank you for your contribution to CockroachDB!