-
Notifications
You must be signed in to change notification settings - Fork 134
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
Saner test event logging default for CI #1576
Conversation
Generate changelog in
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👎 Pretty sure we explicitly wanted to avoid printing out test details instead relying on test reports. Seems like there is an unrelated problem if user tests are taking that long to execute
I've found this kind of logging extremely helpful when a repo has integration tests. Fine to leave it off for unit-tests, but I don't think it's that controversial for integ! |
@ferozco The issue that if all your tests pass, we print out no events at all from the test tasks, so even if each test takes 10 secs but you have 60 of them, the test task will not log anything for 10 mins and the build will be killed by circle. I've seen this happen internally on at least 5 different projects, and since circle just Additionally, it's useful for cases when tests do hang and then eventually get killed by gradle to be able to see which test is bad |
Definitely more of a problem for integ tests than for unit tests, but I've seen this happen in our gradle nebula tests as well (which use the default |
I hear what you're saying perhaps we can find a middle ground? I think printing out every single test case results in a ton of noise and causes CI output to become large (which devtools doesn't like). Can we only print test suites (i.e. classes). Alternatively, we could just recommend the folks with 10m+ tests to configure this themselves |
@ferozco just for extra context, here are the repos in foundry already using this: https://sourcegraph.p.b/search?q=testLogging+repo:foundry/&patternType=literal |
Thanks for the link @iamdanfox I didn't realize how wide spread it was to configure some tests. I think I still stand by my opinion that this configuration is undesirable inside of unit tests though, so I'd be supportive of @CRogers idea of pushing this into dev-env |
// there are lots of tests. Don't do this locally to avoid spamming massive amount of info for people running | ||
// unit tests through the command line | ||
if ("true".equals(System.getenv("CI"))) { | ||
task.getTestLogging() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CRogers can we try to not apply this to the ./gradlew test
tasks which are normally just unit-tests??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I have a commit to do this actually, I was just distracted by testing it on c-v-s that had the silently not running tests problem :p
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed
…ntir/gradle-baseline into callumr/saner-test-logging-defaults
Released 3.58.1 |
Before this PR
We've seen numerous times that when projects have long running integration test tasks that take longer than 10 mins they get killed by circle ("context deadline exceeded"). If there are no failing tests we won't print anything in the gradle output, so it's possible to hit this deadline.
After this PR
==COMMIT_MSG==
Print more test logging output to avoid builds with long running tests getting terminated by circle ("context deadline exceeded").
==COMMIT_MSG==
Possible downsides?
Perhaps more noisy, but the failed tests already have junit xml produced in circle, so this should be strictly better.