SERG-Delft / andy

Andy assesses student's test code. It's used in CSE1110, TU Delft.
MIT License
78 stars 23 forks source link

Investigate why LibraryMetaTestTest is producing extra sysouts #207

Closed mauricioaniche closed 1 year ago

mauricioaniche commented 1 year ago

See for example here: https://github.com/cse1110/andy/actions/runs/5229059665/jobs/9441817798?pr=201

cashbreaker commented 1 year ago

I would like to work on this issue

mauricioaniche commented 1 year ago

It’s yours! We gotta double check it really happens first, though!! 😂

On Fri, 7 Jul 2023 at 16:16, Cashbreaker @.***> wrote:

I would like to work on this issue

— Reply to this email directly, view it on GitHub https://github.com/SERG-Delft/andy/issues/207#issuecomment-1625482744, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAYTTE7ICH443AODTASCXTXPAK4VANCNFSM6AAAAAAZBRQ5BU . You are receiving this because you authored the thread.Message ID: @.***>

--

Maurício Aniche Author of Effective Software Testing: A Developer's Guide https://www.effective-software-testing.com https://www.mauricioaniche.com

cashbreaker commented 1 year ago

I have investigated the issue and "problem" comes from naming of parametrized tests in junit. Their names are by default .toString() of Arguments object. That is why in LibraryMetaTestTest it leads to such huge output.

We can solve the issue by using custom naming of parametrized tests. Here is an example. I have added arguments to @ParametrizedTest annotation and included extra argument called testName.

    @ParameterizedTest(name = "[{index}] insertInLine {0}")
    @MethodSource("insertInLineGenerator")
    void insertInLine(String testName, String oldCode, int lineToInsert, String contentToAdd, String expectedResult) {
        LibraryMetaTest metaTest = (LibraryMetaTest) MetaTest.insertAt("some meta test", lineToInsert, contentToAdd);

        String result = metaTest.evaluate(oldCode);

        assertThat(result)
                .isEqualTo(expectedResult);
    }

    static Stream<Arguments> insertInLineGenerator() {
        return Stream.of(
            // middle
            Arguments.of("middle","line 1\nline 2\nline 3\nline 4\nline 5",
                    3,
                    "extra line 1\nextra line 2",
                    "line 1\nline 2\nextra line 1\nextra line 2\nline 3\nline 4\nline 5"),

            // first
            Arguments.of("first","line 1\nline 2\nline 3\nline 4\nline 5",
                    1,
                    "extra line 1\nextra line 2",
                    "extra line 1\nextra line 2\nline 1\nline 2\nline 3\nline 4\nline 5"),

            // first, but 0
            Arguments.of("first, but 0","line 1\nline 2\nline 3\nline 4\nline 5",
                    0,
                    "extra line 1\nextra line 2",
                    "extra line 1\nextra line 2\nline 1\nline 2\nline 3\nline 4\nline 5"),

            // last line
            Arguments.of("last line","line 1\nline 2\nline 3\nline 4\nline 5",
                    5,
                    "extra line 1\nextra line 2",
                    "line 1\nline 2\nline 3\nline 4\nextra line 1\nextra line 2\nline 5"),

            // end
            Arguments.of("end","line 1\nline 2\nline 3\nline 4\nline 5",
                    6,
                    "extra line 1\nextra line 2",
                    "line 1\nline 2\nline 3\nline 4\nline 5\nextra line 1\nextra line 2"),

            // index bigger than end
            Arguments.of("index bigger than end","line 1\nline 2\nline 3\nline 4\nline 5",
                    7,
                    "extra line 1\nextra line 2",
                    "line 1\nline 2\nline 3\nline 4\nline 5\nextra line 1\nextra line 2")

        );
    }

Here is output of class changed to this scheme

LibraryMetaTest output

Since this is not limited to LibraryMetaTestTest but rather to every class that uses parametrized test I think it would be good idea to use homogeneous scheme of naming throughout the project. @mauricioaniche What do you think about this idea?

cashbreaker commented 1 year ago

While I went throughout the rest of the tests I have also found a CodeSnippetsUtilsTest which has the same problem as the LibraryMetaTestTest

CodeSnippetsUtilsTest output

And addressing my idea about changing the scheme on other classes. For example in a GradeCalculatorTest there is a mix of unit and parametrized tests. In a case a unit test fails it is pretty obvious what goes wrong as the name of the test reflects the problem. However in the case of a parametrized test failure there is only information about arguments that failed, which require to go into the code to find comment what is the purpose of the test.

GradeCalculatorTest output
mauricioaniche commented 1 year ago

And this happens only on the CI, right? I thought it was some leaking sysout! Great that it's just JUnit printing the parameterized tests.

I'd say for the verbose ones, we can override and make the output more friendly.

If this test ever breaks, it's a unit test, super easy to re-run them all locally and see which one broke!

cashbreaker commented 1 year ago

It happens everywhere I think, but IntelliJ does not show \n so it is not a problem there.

I will work on changing the longer ones then and make PR with the changes.