Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

roachtest: add failure message to test if post validations fail #98366

Closed
aliher1911 opened this issue Mar 10, 2023 · 3 comments · Fixed by #103946
Closed

roachtest: add failure message to test if post validations fail #98366

aliher1911 opened this issue Mar 10, 2023 · 3 comments · Fixed by #103946
Assignees
Labels
A-testing Testing tools and infrastructure C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-testeng TestEng Team

Comments

@aliher1911
Copy link
Contributor

aliher1911 commented Mar 10, 2023

In #96949 we added post-test validations for roachtests.
When validation fails GH task is created with an error like

(cluster.go:1387).FailOnInvalidDescriptors: invalid descriptors check failed: pq: query execution canceled due to statement timeout

but linked test log in artifacts end with

11:04:14 test_runner.go:990: tearing down after success; see teardown.log

indicating that test was successful.

It would be helpful to have this failures consistent with "normal" test failures and be visible in artifacts.zip.

Example failure: #97912

Jira issue: CRDB-25228

@aliher1911 aliher1911 added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) A-testing Testing tools and infrastructure T-testeng TestEng Team labels Mar 10, 2023
@blathers-crl
Copy link

blathers-crl bot commented Mar 10, 2023

cc @cockroachdb/test-eng

@blathers-crl blathers-crl bot added this to Triage in Test Engineering Mar 10, 2023
@smg260 smg260 self-assigned this Mar 23, 2023
@smg260
Copy link
Contributor

smg260 commented Mar 29, 2023

Post validations are done during teardown, by which stage we've replaced the test logger with a teardown logger. Possible solutions include

  • move these validations up into runTest, so we can report a failure in the test.log
  • emit a cautionary log statement to check teardown.log for post validation failures

The validation failures should still show up in the artifacts as a failure_x.log.

@aliher1911 just confirming that you expect to see some indication that a post validation failed in test.log, and not just the presence of the failure.log

@aliher1911
Copy link
Contributor Author

Ideally all I want is not to chase errors. In vanilla test failure case I see the error in test log and I can go to the source to see what failed. If there was some remote command involved, I can go to artifacts and find one that failed to see the actual output. With new post validations, there would be no test failure to go to. So I need to look through artifacts to find which one of the failed commands is the real failure. There could be more than one because recovery tests fail some things in successful test runs.

From my point of view having validators in test.log is a good idea as teardown.log is the last place I would go to when trying to fix broken test. But I don't have a big picture.

smg260 pushed a commit to smg260/cockroach that referenced this issue May 26, 2023
smg260 pushed a commit to smg260/cockroach that referenced this issue Jun 6, 2023
craig bot pushed a commit that referenced this issue Jun 12, 2023
103946: roachtest: categorise post-test assertions separately r=smg260 a=smg260

roachtest: categorise post-test assertion errors separately

1. Simple change to mark failures that occur in post-test validation (teardown), so that we can more clearly report a test that has experience a failure after its execution has ended.

2. Every test, unless opted-out, runs various post validations such as node liveness and consistency checks. These checks have been moved to their own function, and failures that occur here are now logged to a separate file, and indicated as such in github when an issue is created.\
\
Previously, these checks were done during teardown and logged to the teardown.log.

Epic: none
Fixes: #98366

Release note: None

Co-authored-by: Miral Gadani <miral@cockroachlabs.com>
@craig craig bot closed this as completed in 1cea00f Jun 12, 2023
Test Engineering automation moved this from Triage to Done Jun 12, 2023
smg260 pushed a commit to smg260/cockroach that referenced this issue Jun 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testing Testing tools and infrastructure C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-testeng TestEng Team
Projects
Development

Successfully merging a pull request may close this issue.

2 participants