Skip to content
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

[sinttest] Allow custom processing of test run result #588

Merged

Conversation

guusdk
Copy link
Member

@guusdk guusdk commented Apr 11, 2024

This adds a new configuration option, testRunResultProcessors, that allows a user to customize the way the results of a test run is processed.

By default, the pre-exising printing-to-stderr is used.

@guusdk guusdk force-pushed the sint_configurable-testresultprocessor branch 2 times, most recently from ae11892 to a34e1af Compare April 11, 2024 14:18
@guusdk guusdk requested a review from Flowdalic April 11, 2024 14:29
Copy link
Member

@Flowdalic Flowdalic left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. Had time to give it some thought. Only minor things to fix.

@guusdk
Copy link
Member Author

guusdk commented Apr 16, 2024

Applied all review feedback.

@guusdk guusdk requested a review from Flowdalic April 16, 2024 14:24
@Flowdalic
Copy link
Member

Squash please.

This adds a new configuration option, `testRunResultProcessors`, that allows a user to customize the way the results of a test run is processed.

By default, the pre-exising printing-to-stderr is used.
@guusdk guusdk force-pushed the sint_configurable-testresultprocessor branch from d35904f to 5622bb0 Compare April 17, 2024 07:40
@guusdk
Copy link
Member Author

guusdk commented Apr 17, 2024

Squashed.

@@ -473,6 +480,15 @@ public Builder setCompatibilityMode(String compatibilityModeString) {
return setCompatibilityMode(compatibilityMode);
}

public Builder setTestRunResultProcessors(String testRunResultProcessorsString) {
if (testRunResultProcessorsString == null) {
Copy link
Member

Choose a reason for hiding this comment

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

Calling a public API like this with null should usually throw an exception. It is not defined that calling this method with null actually does and therefore it is most likely a sign of a programming error, e.g., the user wanting to set a result processor for somehow it ended up being null.

The same argument can be made for the empty string.

Therefore, use StringUtils.requireNonNullNorEmpty(testRunResultProcessorString) here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm happy to change this, but it's a pattern that's used throughout this implementation. Do you want me to change it everywhere?

Copy link
Member

Choose a reason for hiding this comment

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

Uh, fair point. Thanks for pointing this out. There is probably more to it that I don't remember.

In any case, if this is going to get changed, it will be done outside of this PR for all methods.

Copy link
Member

@Flowdalic Flowdalic left a comment

Choose a reason for hiding this comment

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

One minor thing, then its good to go.

@Flowdalic Flowdalic added this pull request to the merge queue Apr 17, 2024
Merged via the queue into igniterealtime:master with commit 8a71029 Apr 17, 2024
3 checks passed
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