roachtest: acceptance subtests interact poorly with teamcity-post-failures.py #30548
Comments
tbg
changed the title
|
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. |
Teach it to 1. group subtests into their parent 2. with the exception of acceptance 3. better test output Touches cockroachdb#30548. See these links for example output: #135 #136 #137 #138 As documentation for future changes to this script, here's how to set it up for testing locally. ``` // export TC_BUILD_ID=918035 // export TC_BUILD_ID=900331 export TC_BUILD_ID=864629 export GITHUB_API_TOKEN=... export TC_BUILD_BRANCH=release-banana export TC_API_PASSWORD=... ``` ```diff diff --git a/build/teamcity-post-failures.py b/build/teamcity-post-failures.py index 229c800c32..f47539340a 100755 --- a/build/teamcity-post-failures.py +++ b/build/teamcity-post-failures.py @@ -24,7 +24,7 @@ BASEURL = "https://teamcity.cockroachdb.com/httpAuth/app/rest/" auth_handler = urllib.request.HTTPBasicAuthHandler() auth_handler.add_password(realm='TeamCity', uri='https://teamcity.cockroachdb.com', - user='robot', + user='tschottdorf', passwd=os.environ['TC_API_PASSWORD']) opener = urllib.request.build_opener(auth_handler) @@ -62,6 +62,8 @@ def collect_build_results(build_id): yield (test_name, test_log, category) def get_probable_milestone(): + # HACK + return None try: tag = subprocess.check_output(['git', 'describe', '--abbrev=0', '--tags'], universal_newlines=True) @@ -128,7 +130,7 @@ Please assign, take a look and update the issue accordingly. def post_issue(issue): req = urllib.request.Request( - 'https://api.github.com/repos/cockroachdb/cockroach/issues', + 'https://api.github.com/repos/tschottdorf/cockroach/issues', data=json.dumps(issue).encode('utf-8'), headers={'Authorization': 'token {0}'.format(os.environ['GITHUB_API_TOKEN'])}) try: ``` Release note: None
30542: security: remove some dead code r=mberhault a=benesch 30549: build: update teamcity-post-failures.py r=benesch a=tschottdorf Teach it to 1. group subtests into their parent 2. with the exception of acceptance 3. better test output Touches #30548. See these links for example output: tbg#135 tbg#136 tbg#137 tbg#138 As documentation for future changes to this script, here's how to set it up for testing locally. ``` // export TC_BUILD_ID=918035 // export TC_BUILD_ID=900331 export TC_BUILD_ID=864629 export GITHUB_API_TOKEN=... export TC_BUILD_BRANCH=release-banana export TC_API_PASSWORD=... ``` ```diff diff --git a/build/teamcity-post-failures.py b/build/teamcity-post-failures.py index 229c800c32..f47539340a 100755 --- a/build/teamcity-post-failures.py +++ b/build/teamcity-post-failures.py @@ -24,7 +24,7 @@ BASEURL = "https://teamcity.cockroachdb.com/httpAuth/app/rest/" auth_handler = urllib.request.HTTPBasicAuthHandler() auth_handler.add_password(realm='TeamCity', uri='https://teamcity.cockroachdb.com', - user='robot', + user='tschottdorf', passwd=os.environ['TC_API_PASSWORD']) opener = urllib.request.build_opener(auth_handler) @@ -62,6 +62,8 @@ def collect_build_results(build_id): yield (test_name, test_log, category) def get_probable_milestone(): + # HACK + return None try: tag = subprocess.check_output(['git', 'describe', '--abbrev=0', '--tags'], universal_newlines=True) @@ -128,7 +130,7 @@ Please assign, take a look and update the issue accordingly. def post_issue(issue): req = urllib.request.Request( - 'https://api.github.com/repos/cockroachdb/cockroach/issues', + 'https://api.github.com/repos/tschottdorf/cockroach/issues', data=json.dumps(issue).encode('utf-8'), headers={'Authorization': 'token {0}'.format(os.environ['GITHUB_API_TOKEN'])}) try: ``` Release note: None Co-authored-by: Nikhil Benesch <nikhil.benesch@gmail.com> Co-authored-by: Tobias Schottdorf <tobias.schottdorf@gmail.com>
|
Couldn't we have |
|
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:
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). |
On the bright side,
Btw, this is less of a concern now that we've broken the most egregiously long running roachtests into smaller pieces (i.e. |
You're not misremembering. I think @tschottdorf's plan is to rewrite that Python script as a simple wrapper around the |
Teach it to 1. group subtests into their parent 2. with the exception of acceptance 3. better test output Touches cockroachdb#30548. See these links for example output: #135 #136 #137 #138 As documentation for future changes to this script, here's how to set it up for testing locally. ``` // export TC_BUILD_ID=918035 // export TC_BUILD_ID=900331 export TC_BUILD_ID=864629 export GITHUB_API_TOKEN=... export TC_BUILD_BRANCH=release-banana export TC_API_PASSWORD=... ``` ```diff diff --git a/build/teamcity-post-failures.py b/build/teamcity-post-failures.py index 229c800c32..f47539340a 100755 --- a/build/teamcity-post-failures.py +++ b/build/teamcity-post-failures.py @@ -24,7 +24,7 @@ BASEURL = "https://teamcity.cockroachdb.com/httpAuth/app/rest/" auth_handler = urllib.request.HTTPBasicAuthHandler() auth_handler.add_password(realm='TeamCity', uri='https://teamcity.cockroachdb.com', - user='robot', + user='tschottdorf', passwd=os.environ['TC_API_PASSWORD']) opener = urllib.request.build_opener(auth_handler) @@ -62,6 +62,8 @@ def collect_build_results(build_id): yield (test_name, test_log, category) def get_probable_milestone(): + # HACK + return None try: tag = subprocess.check_output(['git', 'describe', '--abbrev=0', '--tags'], universal_newlines=True) @@ -128,7 +130,7 @@ Please assign, take a look and update the issue accordingly. def post_issue(issue): req = urllib.request.Request( - 'https://api.github.com/repos/cockroachdb/cockroach/issues', + 'https://api.github.com/repos/tschottdorf/cockroach/issues', data=json.dumps(issue).encode('utf-8'), headers={'Authorization': 'token {0}'.format(os.environ['GITHUB_API_TOKEN'])}) try: ``` Release note: None
This code looked bad before and it hasn't gotten any better, but this should prevent new bogus issues like cockroachdb#30595. There's discussion in cockroachdb#30548 on revamping this, but right now band aid is what's on the menu. Release note: None
30629: build: in python issue poster, ignore roachtest/acceptance r=petermattis a=tschottdorf This code looked bad before and it hasn't gotten any better, but this should prevent new bogus issues like #30595. There's discussion in #30548 on revamping this, but right now band aid is what's on the menu. Closes #30595. Release note: None Co-authored-by: Tobias Schottdorf <tobias.schottdorf@gmail.com>
This code looked bad before and it hasn't gotten any better, but this should prevent new bogus issues like cockroachdb#30595. There's discussion in cockroachdb#30548 on revamping this, but right now band aid is what's on the menu. Release note: None
knz
added
the
C-enhancement
|
@andreimatei Tossing your way because I believe your plan is to get rid of subtests. |
|
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. 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. |
|
We have marked this issue as stale because it has been inactive for |
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:
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
The text was updated successfully, but these errors were encountered: