Skip to content

Conversation

@hobovsky
Copy link
Contributor

@hobovsky hobovsky commented Feb 1, 2023

Fixes codewars/runner#230 .

Example test suite which showcases various possible test outcomes:

import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.ValueSource;

import static org.junit.jupiter.api.Assertions.assertEquals;
class SampleTests {

    @ParameterizedTest(name = "{0}")
    @ValueSource(strings = {"a", "x", "b"})
    void testSuccess(String s) {
        System.out.println(">>"+s+"<<");
        assertEquals(s, s);
    }


    @ParameterizedTest(name = "{0}")
    @ValueSource(strings = {"a", "x", "b"})
    void testFailingAssertion(String s) {
        System.out.println(">>"+s+"<<");
        assertEquals(s, s + s, "Incorrect answer for input: " + s);
    }

    @ParameterizedTest(name = "Test \"{0}\"")
    @ValueSource(strings = {"a", "", "b"})
    void testThrowing(String s) {
        System.out.println(">>"+s+"<<");
        assertEquals(s.charAt(0), s.charAt(0), "Incorrect answer for input: " + s);
    }

    @ParameterizedTest(name = "{0}")
    @ValueSource(strings = {"a", "", "b"})
    void testCrash(String s) {
        System.out.println(">>"+s+"<<");
        assertEquals(s, s);
    }
}

Copy link
Member

@kazk kazk left a comment

Choose a reason for hiding this comment

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

I don't mind changing to use 4 spaces, but can you commit separately?

} else {
String formattedMessage = formatMessage(msg);
String formattedStackTrace = formatMessage(readStackTrace(throwable));
System.out.printf("\n<ERROR::>%s<:LF:><:LF:>Stack trace:<:LF:><:LF:>%s\n", formattedMessage, formattedStackTrace);
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason not to collapse the stack trace like above (L176)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well my reasoning was that a collapsed stacktrace does not necessarily represent a (potentially severe) problem with runtime but more a failing test case. I thought that since it's an internal error of JUnit, it deserves to crash hard. But it's just what I thought, can change it to whatever you prefer.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense. Can you still put them in a separate container? Just don't add -. I think that's more readable.

Copy link
Contributor Author

@hobovsky hobovsky Feb 1, 2023

Choose a reason for hiding this comment

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

Here it is. I am not sure I like how the stacktrace now looks logically separate from the error, but if you prefer it this way, then fine.
BTW stacktraces in log panel look much better than in the ERROR:: panel due to lack of excessive line wrapping. Would it be possible to make the error panel behave in similar way as log panels? Maybe this would also solve the readability issue.

Copy link
Member

Choose a reason for hiding this comment

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

Can you post a screenshot? I saw the original on Discord, but can you also repost that here, so it's easier to compare?

I'll look into the excessive line wrapping.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what screenshot exactly you want? Here's the ERROR with LOG vs a single ERROR:

image

image

Is this what you asked?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For comparison, here is with CSS property white-space: pre:

image

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, the last one might be the best. I think "Stack Trace:" can be removed.

I'll fix the CSS and let you know.

Copy link
Member

Choose a reason for hiding this comment

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

Deployed the CSS change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a reason not to collapse the stack trace like above (L176)?

Now I see what you mean, and it was a copy/paste mistake that outputError used collapsible LOG:: in one branch, and ERROR:: in another. I changed both branches in outputError to use ERROR:: and I think that after your fix to CSS, it looks good.

outputFailure uses collapsed LOG:: as it has been all the time.

@hobovsky
Copy link
Contributor Author

hobovsky commented Feb 1, 2023

I don't mind changing to use 4 spaces, but can you commit separately?

This was unintentional, IDE did this. I did notice the large diff, but I thought these were Windows line breaks. I will try to revert the change in whitespace.

@kazk kazk changed the title JUnit5 ParameterizedTests suppress PreconditionViolationException and… Fix ParameterizedTests suppressing PreconditionViolationException and not failing the tests Feb 1, 2023
@hobovsky hobovsky requested a review from kazk February 1, 2023 23:54
@hobovsky
Copy link
Contributor Author

hobovsky commented Feb 2, 2023

For comparison, this is how test result looks in IDE runner:

image

@kazk kazk changed the title Fix ParameterizedTests suppressing PreconditionViolationException and not failing the tests Fix ParameterizedTests suppressing PreconditionViolationException Feb 2, 2023
@kazk kazk merged commit 4feace8 into codewars:main Feb 2, 2023
@hobovsky hobovsky deleted the fix/missing-PreconditionViolationException branch February 2, 2023 20: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.

Java: JUnit5 ParameterizedTests suppress PreconditionViolationException and don't fail the tests

2 participants