feat(#4833): add logging for progress of parameterized tests in EoSyntaxTest #4834
Conversation
📝 WalkthroughWalkthroughAdds a JUnit 5 test extension LogProgress, applies it to EoSyntaxTest via Changes
Sequence Diagram(s)sequenceDiagram
participant JUnit as JUnit 5
participant Test as Test Method
participant Extension as LogProgress
participant Logger as org.jcabi.log.Logger
JUnit->>Test: execute()
Test->>JUnit: completes
JUnit->>Extension: afterEach(context)
Extension->>Extension: inspect test method metadata
alt method annotated with `@ParameterizedTest`
Extension->>Logger: log "parameterized test completed" (INFO)
end
Extension->>JUnit: callback finished
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~10 minutes 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
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 introduces a JUnit extension to log the progress of parameterized tests in EoSyntaxTest, addressing test execution issues where tests appear to hang during mvn clean install. The extension logs completion messages for each parameterized test iteration to help diagnose test progress.
Changes:
- Added
LogProgressextension class to log parameterized test completion - Applied the
LogProgressextension toEoSyntaxTestclass
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| eo-parser/src/test/java/org/eolang/parser/LogProgress.java | New JUnit extension that logs completion of parameterized test executions to help diagnose test hanging issues |
| eo-parser/src/test/java/org/eolang/parser/EoSyntaxTest.java | Applied the LogProgress extension to enable progress logging for parameterized tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (LogProgress.parameterized(context)) { | ||
| Logger.info( | ||
| clazz, | ||
| "Parameterized test %s done", | ||
| context.getDisplayName().lines().findFirst().orElse(context.getUniqueId()) | ||
| ); | ||
| } | ||
| logger.setLevel(before); |
There was a problem hiding this comment.
The logger level is modified on line 27 and restored on line 35, but if an exception occurs during the logging operation (lines 28-34), the logger level will not be restored. Consider using a try-finally block to ensure the logger level is always restored, regardless of exceptions.
| if (LogProgress.parameterized(context)) { | |
| Logger.info( | |
| clazz, | |
| "Parameterized test %s done", | |
| context.getDisplayName().lines().findFirst().orElse(context.getUniqueId()) | |
| ); | |
| } | |
| logger.setLevel(before); | |
| try { | |
| if (LogProgress.parameterized(context)) { | |
| Logger.info( | |
| clazz, | |
| "Parameterized test %s done", | |
| context.getDisplayName().lines().findFirst().orElse(context.getUniqueId()) | |
| ); | |
| } | |
| } finally { | |
| logger.setLevel(before); | |
| } |
| final var clazz = context.getTestClass().orElse(LogProgress.class); | ||
| final org.apache.log4j.Logger logger = org.apache.log4j.Logger.getLogger(clazz); | ||
| final var before = logger.getLevel(); |
There was a problem hiding this comment.
The use of 'var' for type inference deviates from the codebase convention. Throughout the codebase, explicit types are used for variable declarations. For consistency, declare these variables with explicit types: 'final Class<?> clazz' and 'final Level before'.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@eo-parser/src/test/java/org/eolang/parser/LogProgress.java`:
- Around line 24-26: The code uses Java's local-variable type inference (`var`)
which isn't supported by the project's Java target; replace `var` with explicit
types for the variables in the LogProgress test setup: change the declaration of
`clazz` to `Class<?> clazz = context.getTestClass().orElse(LogProgress.class);`
and change `before` to the explicit type returned by `logger.getLevel()` (e.g.,
`org.apache.log4j.Level before = logger.getLevel();`), keeping the existing
logger declaration `org.apache.log4j.Logger logger =
org.apache.log4j.Logger.getLogger(clazz);` and usages unchanged.
- Around line 26-35: logger.getLevel() can be null and calling
logger.setLevel(null) will clear inherited behavior; modify the LogProgress code
so you capture the effective level instead and always restore it in a finally
block: use logger.getEffectiveLevel() (or fall back to a non-null Level) to set
the temporary Level.INFO, and wrap the logging and setLevel calls in try { ... }
finally { logger.setLevel(originalEffectiveLevel); } referencing the logger
variable and the LogProgress class/method where getLevel()/setLevel() are used.
🧹 Nitpick comments (1)
eo-parser/src/test/java/org/eolang/parser/EoSyntaxTest.java (1)
56-76: Consider consolidating log level management.The test class now has two mechanisms managing log levels:
- The new
LogProgressextension (for the test class logger)- The existing
setUp/tearDownmethods (lines 66-76, forEoSyntax.classlogger)This is fine since they target different loggers, but consider whether the existing
@BeforeEach/@AfterEachlogic could be moved intoLogProgressor a similar extension for consistency, especially if other test classes need the same pattern.
| final var before = logger.getLevel(); | ||
| logger.setLevel(Level.INFO); | ||
| if (LogProgress.parameterized(context)) { | ||
| Logger.info( | ||
| clazz, | ||
| "Parameterized test %s done", | ||
| context.getDisplayName().lines().findFirst().orElse(context.getUniqueId()) | ||
| ); | ||
| } | ||
| logger.setLevel(before); |
There was a problem hiding this comment.
Handle potential null value from getLevel().
logger.getLevel() returns null when no level is explicitly configured on the logger (it inherits from parent). Passing null to setLevel() on line 35 will clear the level, which may differ from the original inherited behavior.
🛡️ Proposed fix to handle null level
public void afterEach(final ExtensionContext context) {
- final var clazz = context.getTestClass().orElse(LogProgress.class);
+ final Class<?> clazz = context.getTestClass().orElse(LogProgress.class);
final org.apache.log4j.Logger logger = org.apache.log4j.Logger.getLogger(clazz);
- final var before = logger.getLevel();
- logger.setLevel(Level.INFO);
- if (LogProgress.parameterized(context)) {
- Logger.info(
- clazz,
- "Parameterized test %s done",
- context.getDisplayName().lines().findFirst().orElse(context.getUniqueId())
- );
+ final Level before = logger.getEffectiveLevel();
+ try {
+ logger.setLevel(Level.INFO);
+ if (LogProgress.parameterized(context)) {
+ Logger.info(
+ clazz,
+ "Parameterized test %s done",
+ context.getDisplayName().lines().findFirst().orElse(context.getUniqueId())
+ );
+ }
+ } finally {
+ logger.setLevel(before);
}
- logger.setLevel(before);
}Using getEffectiveLevel() returns the inherited level if none is set. Wrapping in try-finally ensures level is restored even if logging throws.
🤖 Prompt for AI Agents
In `@eo-parser/src/test/java/org/eolang/parser/LogProgress.java` around lines 26 -
35, logger.getLevel() can be null and calling logger.setLevel(null) will clear
inherited behavior; modify the LogProgress code so you capture the effective
level instead and always restore it in a finally block: use
logger.getEffectiveLevel() (or fall back to a non-null Level) to set the
temporary Level.INFO, and wrap the logging and setLevel calls in try { ... }
finally { logger.setLevel(originalEffectiveLevel); } referencing the logger
variable and the LogProgress class/method where getLevel()/setLevel() are used.
|
Now in tests I might perfectly see the following output: Instead of silence. |
volodya-lombrozo
force-pushed
the
4833-eo-parser-test-halt
branch
from
9748995 to
e5bd471
Compare
🚀 Performance AnalysisAll benchmarks are within the acceptable range. No critical degradation detected (threshold is 100%). Please refer to the detailed report for more information. Click to see the detailed report
✅ Performance gain: |
|
@yegor256 could you have a look, please? I'm not sure if it's a perfect solution, but now I can see some progress in logs |
| final Class<?> clazz = context.getTestClass().orElse(LogProgress.class); | ||
| final org.apache.log4j.Logger logger = org.apache.log4j.Logger.getLogger(clazz); | ||
| final Level before = logger.getLevel(); | ||
| logger.setLevel(Level.INFO); |
There was a problem hiding this comment.
@volodya-lombrozo this indeed is dirty. Can't you just set this log level in the src/test/resources/log4j.properties? The effect should be the same.
There was a problem hiding this comment.
@yegor256 agree. Fixed. Could you take a look one more time, please?
… and update config
|
@volodya-lombrozo Great work on your contribution! 🎯 You've earned +16 points and your total score now stands at +208. Keep up the momentum with quality contributions, and don't forget to monitor your progress through your Zerocracy account. |
|
@yegor256 Hey! Nice work on that review 👍 You snagged +7 points this time: started with the standard +12 base points, but got dinged -5 for having just 1 comment (our policy encourages at least 12 comments to avoid penalties). Your running total is now +140 - keep those detailed reviews coming to maximize your bonuses! Don't forget to peek at your Zerocracy account when you get a chance. |
3 participants
This PR introduces the
LogProgressextension to enhance logging for parameterized tests ineo-parser, addressing test execution issues duringmvn clean install.Fixes #4833
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.