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

chore(Java): disable verbose unit test logging #948

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

texastony
Copy link
Contributor

@texastony texastony commented Nov 3, 2024

Issue #, if available:

Description of changes:

Disable Verbose test logging to make the LocalCMCTests less flaky.
It is hard to log 300k messages to a terminal,
particularly if 10 threads are concurrently writing to the terminal.

This MAY reduce how often we encounter a LocalCMCTests failure that appears like:

Execution failed for task ':test'.
> Received a completed event for test with unknown id '2.25506'. Registered test ids: '[2.2, 2.3, 2.1, :test]'

The theory is that by removing the event lifecycle handler from the concurrency testing,
each of the 300k tests completes without triggering the handler.

The assumption is that the event lifecycle handler is a cause for concurrent testing failure.

Squash/merge commit message, if applicable:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@texastony texastony requested a review from a team as a code owner November 3, 2024 22:56
Comment on lines +227 to +241
// Disable Verbose test logging to make the LocalCMCTests less flaky.
// It is hard to log 300k messages to a terminal,
// particularly if 10 threads are concurrently writing to the terminal.
// testLogging {
// lifecycle {
// events = mutableSetOf(org.gradle.api.tasks.testing.logging.TestLogEvent.FAILED, org.gradle.api.tasks.testing.logging.TestLogEvent.PASSED, org.gradle.api.tasks.testing.logging.TestLogEvent.SKIPPED)
// exceptionFormat = org.gradle.api.tasks.testing.logging.TestExceptionFormat.FULL
// showExceptions = true
// showCauses = true
// showStackTraces = true
// showStandardStreams = true
// }
// info.events = lifecycle.events
// info.exceptionFormat = lifecycle.exceptionFormat
// }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just remove this instead of commenting?

Copy link
Contributor Author

@texastony texastony Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because if a test fails,
it is very helpful to know what it is.
Which this lifecycle logging helps with.

Locally, the logs are not that helpful,
as TestNG will write a test report HTML.

But that report is not available from GitHub CI.
Only the logs are.
Hence, it is nice to leave this comment block in case someone needs it in the future.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then maybe use a flag or disable the params that control this? Maybe this can help: https://docs.gradle.org/current/javadoc/org/gradle/api/tasks/testing/logging/TestExceptionFormat.html#SHORT

I'm afraid that by commenting it out, we are creating a precedence that it's okay to comment out code for "future" use case. And have seen this in our code base. If nothing else, we can remove this and add link to the gradle doc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants