Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis change introduces static caches for lint name and lint collections, refactors XSL-based lint loading to use a cached list built via resource resolution, and narrows test lint scopes to speed and stabilize test execution. Tests and a cactoos dependency version were updated. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR targets issue #811 by reducing full test-suite runtime via minimizing repeated lint initialization work and narrowing down which lints are exercised in some tests.
Changes:
- Introduces a “single lint” fast path in
SourceTestso several tests validate defect detection without running the full lint set. - Adds static caching for mono/WPA lint name collections to avoid recomputing them during WPA/Program setup.
- Caches XSL-based lint instances in
PkByXslto avoid repeated classpath scanning and XSL parsing.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/test/java/org/eolang/lints/SourceTest.java |
Uses a single XSL lint in selected tests to reduce runtime; adjusts test inputs accordingly. |
src/main/java/org/eolang/lints/WpaLintNames.java |
Caches WPA lint names statically to reduce repeated iteration. |
src/main/java/org/eolang/lints/PkWpa.java |
Reuses cached mono lint names when building WPA lints. |
src/main/java/org/eolang/lints/PkByXsl.java |
Loads and caches XSL-based lint instances once, then shuffles the cached list per iteration. |
src/main/java/org/eolang/lints/MonoLints.java |
Precomputes a cached list of all lint names used by LtIncorrectUnlint. |
src/main/java/org/eolang/lints/MonoLintNames.java |
Caches mono lint names statically to reduce repeated iteration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -133,24 +142,24 @@ void suppressesManyLints() throws IOException { | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
The @Timeout annotation was removed from this test. Dropping timeouts usually doesn’t make the suite faster, but it does remove protection against hangs (e.g., parser/XSLT deadlocks or CI environment stalls). Consider keeping a timeout here (potentially reduced if the goal is faster feedback) so a single stuck run can’t block the whole test suite.
| @Timeout(unit = TimeUnit.SECONDS, value = 60L) |
| ); | ||
| } | ||
|
|
||
| @Tag("deep") |
There was a problem hiding this comment.
The @Timeout for this multithreaded test was removed. Since this test runs in parallel and is tagged "deep", keeping a timeout is important to avoid the entire suite hanging indefinitely on rare CI stalls or deadlocks; consider restoring a reasonable timeout (even if shorter than before).
| @Tag("deep") | |
| @Tag("deep") | |
| @Timeout(value = 1, unit = TimeUnit.MINUTES) |
| throw new IllegalArgumentException(ex); | ||
| } | ||
| } | ||
| ).collect(Collectors.toList()); | ||
| } catch (final IOException ex) { | ||
| throw new IllegalArgumentException(ex); |
There was a problem hiding this comment.
IOException is wrapped into IllegalArgumentException without any context about which XSL resource failed to load. This makes failures hard to diagnose (especially during static initialization). Consider using an exception type that reflects an internal loading failure (e.g., IllegalStateException) and include the resource URL/filename in the message for both the inner and outer catch paths.
| throw new IllegalArgumentException(ex); | |
| } | |
| } | |
| ).collect(Collectors.toList()); | |
| } catch (final IOException ex) { | |
| throw new IllegalArgumentException(ex); | |
| throw new IllegalStateException( | |
| String.format( | |
| "Failed to load XSL lint resource from '%s'", | |
| res.toString() | |
| ), | |
| ex | |
| ); | |
| } | |
| } | |
| ).collect(Collectors.toList()); | |
| } catch (final IOException ex) { | |
| throw new IllegalStateException( | |
| "Failed to resolve XSL lint resources with pattern 'classpath*:org/eolang/lints/**/*.xsl'", | |
| ex | |
| ); |
github-actions
bot
added
the
core
|
@yegor256 Hey there! Nice work on the contribution - you've earned +4 points this time! Here's the breakdown: +16 as a basis, +8.45 for your 169 hits-of-code, -4 for going over 100 hits-of-code, and -8 for skipping the code review (ouch!). Next time, try to get some reviews in there to avoid that penalty and maybe keep the contributions a bit more focused - you'll hit those higher point ranges! Your running score is +5, so keep the momentum going and don't forget to check your Zerocracy account too! 🚀 |
fixes #811
Summary by CodeRabbit
Refactor
Tests
Chores