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

Make it easier to find test failures in CI output #2511

Merged
merged 4 commits into from
Oct 14, 2023

Conversation

swalchemist
Copy link
Contributor

@swalchemist swalchemist commented Sep 30, 2023

Searching for "FAILURE" in the CI output in a browser (case insensitive) to find failed tests always finds things that aren't test failures. This PR applies the principle of never logging the word "failure" except when JUnit is reporting a test failure.

"Fail" is still ok to include in method names and other output.

Two additional refactors in the affected code are also included.

* Searching for "FAILED" in the CI output always found this method, whether it failed or not.
* JUnit 5 has a better way to assert on an expected exception.
* Cleaned up an unncessary type specification.
* Fixed a typo.
* Cleaned up an import.
* Trying to make it easier to search for "FAILED" in the CI output to find failed tests.
* Also tried to make the message easier to understand.
@cf-gitbot
Copy link

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/186153618

The labels on this github issue will be updated when the story is started.

Copy link
Contributor

@hsinn0 hsinn0 left a comment

Choose a reason for hiding this comment

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

I do not see much usefulness of this PR. We can just search for "FAILED" case-sensitive, can't we?

@swalchemist
Copy link
Contributor Author

I do not see much usefulness of this PR. We can just search for "FAILED" case-sensitive, can't we?

Web browsers make it difficult to do a case-sensitive search.

Also, there are a few refactors here that I think are worthwhile.

Copy link
Contributor

@bruce-ricard bruce-ricard left a comment

Choose a reason for hiding this comment

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

I like the idea for the change. Thank you for doing this.

I myself committed a similar change several years ago.

Mostly requesting changing because of the FAILURE commit that I don't understand.

* The meaning of the name "retryFailOnPassedAfterRetry" was difficult to understand. It causes the full test run to fail when a flaky test is detected (both passes and fails on multiple runs).
* Seems the indirection is required, first setting and checking a variable, then setting the "failOnPassedAfterRetry" property used by the retry plugin.
@swalchemist swalchemist force-pushed the refactor-fail-output branch from f792b9d to 531db67 Compare October 3, 2023 01:14
@swalchemist swalchemist merged commit cb2d41f into develop Oct 14, 2023
17 of 18 checks passed
@swalchemist swalchemist deleted the refactor-fail-output branch October 14, 2023 02:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

4 participants