Skip to content

Review of framework-test package#1724

Merged
wmdietl merged 9 commits into
masterfrom
test-review
May 16, 2026
Merged

Review of framework-test package#1724
wmdietl merged 9 commits into
masterfrom
test-review

Conversation

@wmdietl

@wmdietl wmdietl commented May 16, 2026

Copy link
Copy Markdown
Member

No description provided.

Copilot AI review requested due to automatic review settings May 16, 2026 20:09

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Small cleanup and bug-fix pass over the framework-test package: tightens collection construction, replaces Scanner with BufferedReader, removes a now-unused helper, and fixes a couple of latent correctness issues in diagnostic parsing and equality.

Changes:

  • Replace Scanner-based file scanning with Files.newBufferedReader, and add a skip-test fast-path filter in TestUtilities.isJavaTestFile.
  • Defensive-copy diagnosticFiles, mark processors unmodifiable in ImmutableTestConfiguration; simplify list mutation in TestConfigurationBuilder and drop the unused catListAndIterable helper; switch String.join("%n", ...) to System.lineSeparator().
  • Fix TestDiagnosticUtils.fromPatternMatching to read from warningMatcher (not diagnosticMatcher) in the warning branch, and make TestDiagnostic.equals use this.getClass() so DetailedTestDiagnostic.equals (which delegates via super) actually works.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
framework-test/.../TypecheckResult.java Build unexpectedDiagnostics directly from actualDiagnostics; wrap expected in a HashSet for removeAll.
framework-test/.../TypecheckExecutor.java Use Collections.emptyList() instead of an empty ArrayList for the classes argument.
framework-test/.../TestUtilities.java Switch Scanner to BufferedReader; add skip-test fast-path to short-circuit per-line checks.
framework-test/.../TestConfigurationBuilder.java Inline list appends, drop catListAndIterable, and use System.lineSeparator() for joining error lines.
framework-test/.../SimpleOptionMap.java Minor: drop redundant toAppend.length() argument to substring.
framework-test/.../PerDirectorySuite.java Use real newlines in error message; construct ctor args array directly.
framework-test/.../ImmutableTestConfiguration.java Defensive-copy diagnosticFiles; make processors an unmodifiable list.
framework-test/.../diagnostics/TestDiagnosticUtils.java Read the linenogroup/lineno from warningMatcher and guard for patterns without that named group.
framework-test/.../diagnostics/TestDiagnostic.java Use this.getClass() in equals so subclasses (e.g. DetailedTestDiagnostic) can be equal.
framework-test/.../CheckerFrameworkWPIPerDirectoryTest.java Switch Scanner to BufferedReader in hasSkipComment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +154 to +159
// Wrap expectedDiagnostics in a HashSet so the two removeAll calls below get O(1)
// membership tests.
Set<TestDiagnostic> expectedSet = new HashSet<>(expectedDiagnostics);

Set<TestDiagnostic> unexpectedDiagnostics = new LinkedHashSet<>(actualDiagnostics);
unexpectedDiagnostics.removeAll(expectedSet);
wmdietl and others added 2 commits May 16, 2026 16:41
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Werner Dietl <wdietl@gmail.com>
@wmdietl wmdietl merged commit 0146116 into master May 16, 2026
50 checks passed
@wmdietl wmdietl deleted the test-review branch May 16, 2026 22:37
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.

2 participants