-
Notifications
You must be signed in to change notification settings - Fork 96
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
Implementing mandatory uncaught exception logging on loader tasks. #553
base: master
Are you sure you want to change the base?
Conversation
4afdf96
to
6e29923
Compare
private static final Logger LOG = LoggerFactory.getLogger(LoadTask.class); | ||
public static final String LOAD_TASK_ERROR_FORMAT = "Exception while running %s: %s"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move it to ErrorMessageFormat
?
|
||
|
||
def "Test error gets logged on failed test"() { | ||
setup: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
duplicate
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove extra line
def "Test error gets logged on failed test"() { | ||
setup: | ||
setup: "Instantiate a task scheduler" | ||
ObjectMapper MAPPER = new ObjectMappersSuite().getMapper() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like MAPPER
is unused
setup: "Instantiate a task scheduler" | ||
ObjectMapper MAPPER = new ObjectMappersSuite().getMapper() | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove extra line
|
||
class FailingTestLoadTask extends LoadTaskSpec.OneOffTestLoadTask { | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove extra line
CHANGELOG.md
Outdated
@@ -74,6 +74,11 @@ Current | |||
|
|||
|
|||
### Changed: | |||
|
|||
- [Added mandatory logging for LoadTask errors] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing PR link
|
||
TaskScheduler scheduler = new TaskScheduler(2) | ||
LoadTask<Boolean> loader = new FailingTestLoadTask(expectedDelay) | ||
loader.setFuture( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can unwrap
scheduler.schedule( | ||
loader, | ||
1, | ||
TimeUnit.MILLISECONDS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can unwrap
|
||
expect: | ||
loader.failed | ||
logAppender.getMessages().find { it.contains(/UNIQUE_VALUE_1/) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change "UNIQUE_VALUE_1"
to FailingTestLoadTask.UNIQUE_VALUE_1
for better maintainability?
a8006ad
to
8a56af3
Compare
8a56af3
to
46a55a5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Adding error logging on loader tasks.