CompilationTestHelper reports only the first line that fails its expectation
- Dominant language
- Java
- Stars
- 7.2k
- Forks
- 820
- Avg merge
- 5h 9m
- Merged PRs (30d)
- 50
Description
When several lines of a source given to `CompilationTestHelper.addSourceLines` fail their expectations, the test failure names only the first of them. The next one is reported only after the first is fixed and the test runs again.
## Reproducer
Error Prone 2.50.0 (`error_prone_core` and `error_prone_test_helpers`), JUnit 4.13.2, OpenJDK 21.0.9, Maven 3.9.16.
```java
package repro;
import com.google.errorprone.CompilationTestHelper;
import com.google.errorprone.bugpatterns.DeadException;
import org.junit.Test;
public class FirstMismatchTest {
private final CompilationTestHelper helper =
CompilationTestHelper.newInstance(DeadException.class, getClass());
@Test
public void twoMarkersWithoutDiagnostics() {
helper
.addSourceLines(
"Test.java",
"""
class Test {
void first() {
// BUG: Diagnostic contains: DeadException
int a = 1;
}
void second() {
// BUG: Diagnostic contains: DeadException
int b = 2;
}
}
""")
.doTest();
}
@Test
public void twoDiagnosticsWithoutMarkers() {
helper
.addSourceLines(
"Test.java",
"""
class Test {
void first() {
new RuntimeException();
}
void second() {
new IllegalStateException();
}
}
""")
.doTest();
}
}
```
`mvn test` prints, for the first test:
```text
com.google.common.truth.AssertionErrorWithFacts:
Did not see an error on line 4 matching DeadException. There were no errors.
expected to be true
at repro.FirstMismatchTest.twoMarkersWithoutDiagnostics(FirstMismatchTest.java:28)
```
Line 8 carries a marker and has no diagnostic either, and the failure does not mention it.
For the second test:
```text
java.lang.AssertionError:
Saw unexpected error on line 3. All errors:
/Test.java:3: error: [DeadException] Exception created but not thrown
new RuntimeException();
^
(see https://errorprone.info/bugpattern/DeadException)
Did you mean 'throw new RuntimeException();'?
/Test.java:6: error: [DeadException] Exception created but not thrown
new IllegalStateException();
^
(see https://errorprone.info/bugpattern/DeadException)
Did you mean 'throw new IllegalStateException();'?
at repro.FirstMismatchTest.twoDiagnosticsWithoutMarkers(FirstMismatchTest.java:46)
```
The diagnostic on line 6 is in the list of all errors, but the failure reports only line 3 as a mismatch. To find the other mismatches, the reader compares every listed diagnostic with the markers in the source, line by line.
## Where it stops
`DiagnosticTestHelper.assertHasDiagnosticOnAllMatchingLines` reads the source one line at a time and throws at the first line that does not match: through `assertWithMessage(...).isTrue()` for a missing diagnostic ([L264-268](https://github.com/google/error-prone/blob/v2.50.0/test_helpers/src/main/java/com/google/errorprone/DiagnosticTestHelper.java#L264-L268)) or a missing check name ([L277-281](https://github.com/google/error-prone/blob/v2.50.0/test_helpers/src/main/java/com/google/errorprone/DiagnosticTestHelper.java#L277-L281)), and through `fail` for an unexpected one ([L289](https://github.com/google/error-prone/blob/v2.50.0/test_helpers/src/main/java/com/google/errorprone/DiagnosticTestHelper.java#L289)). The file is the same on `master` today.
## Who runs into it
Any test whose source holds more than one expectation, when a change to the check breaks two of them at once. A rough regex count over `core/src/test/java` finds 201 of 5177 `addSourceLines` calls with two or more `// BUG: Diagnostic` markers. That count leaves out sources with one marker and several lines expected to stay clean, which can fail on two lines as well.
It came up in the review of uber/NullAway#1834. A reviewer asked for a case that NullAway reports and its unreported counterpart to share one source instead of two copies of it, so a reader sees the one line in which they differ. With the helper as it is, the price of that layout is a report that hides the second failure whenever one change breaks both lines.
## Expected
One failure that lists every line that failed its expectation, each with the message the helper prints for it today. For the first test that means both line 4 and line 8; for the second, both line 3 and line 6.
One possible shape, for the first test:
```text
2 lines did not match their expectations:
Did not see an error on line 4 matching DeadException.
Did not see an error on line 8 matching DeadException.
There were no errors.
```
The wording and layout are yours to choose. What I am asking for is that no mismatched line is left out of the failure. This is a request for a more useful report rather than a defect against a documented contract: the Javadoc of `addSourceLines` says what is checked on every line, not how failures are reported. Go's `analysistest`, for comparison, reports each unexpected and each missing diagnostic through its own `t.Errorf` ([analysistest.go](https://github.com/golang/tools/blob/52dd431b00a0be368e5a813530ff63fcf5eb8e59/go/analysis/analysistest/analysistest.go#L654-L714)).
I searched the tracker for `CompilationTestHelper`, `DiagnosticTestHelper`, and the text of both failure messages, and found no earlier report.
I can send the pull request: collect the messages inside the loop and fail once after it, with each message unchanged.
Contributor guide
Research direction
Start in test_helpers/src/main/java/com/google/errorprone/DiagnosticTestHelper.java, especially assertHasDiagnosticOnAllMatchingLines and the cited failure paths. Add regression coverage for multiple missing and unexpected diagnostics, then run the test-helper Maven tests. Done means one failure reports every mismatched source line while preserving each current message.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- java
- Domain
- testing
- Issue type
- Bug
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 86/100