Why are the existing AB3 tests overly indirect?

[E0735389]

e.g. one of the test looks like this

        Person expectedPerson = new PersonBuilder(BOB).withTags(VALID_TAG_FRIEND).build();

        // whitespace only preamble
        assertParseSuccess(parser, PREAMBLE_WHITESPACE + NAME_DESC_BOB + PHONE_DESC_BOB + EMAIL_DESC_BOB
                + ADDRESS_DESC_BOB + TAG_DESC_FRIEND, new AddCommand(expectedPerson));

        // multiple tags - all accepted
        Person expectedPersonMultipleTags = new PersonBuilder(BOB).withTags(VALID_TAG_FRIEND, VALID_TAG_HUSBAND)
                .build();
        assertParseSuccess(parser,
                NAME_DESC_BOB + PHONE_DESC_BOB + EMAIL_DESC_BOB + ADDRESS_DESC_BOB + TAG_DESC_HUSBAND + TAG_DESC_FRIEND,
                new AddCommand(expectedPersonMultipleTags));

where

    public static final String NAME_DESC_BOB = " " + PREFIX_NAME + VALID_NAME_BOB;

and PREFIX_NAME is outright imported from the real code by import static seedu.address.logic.parser.CliSyntax.PREFIX_NAME;.

The point of tests is to ensure that the main code does not have bugs. To do that, we want the tests to not have bugs as well.

However, it appears to me that the more complicated-looking the tests are, the more likely they are to have a hidden bug and/or test the wrong thing. Why don't we directly hard code the string that consists of the test into the test code? That would reduce the probability of accidentally testing the wrong thing.

[damithc]

@E0735389 This is a fair observation. Others feel free to chime in with your views.

In the meantime also feel free to simplify/improve tests (or any other code) as you see fit. You can also experiment by changing the current code in a way you think will improve them. If it turns out it made things worse, you can always go back to the original approach, or try yet another approach.

[aureliony]

There is an extensibility trade-off if you were to hardcode the strings directly in the tests.

For exmaple, if the PREFIX were to change in the future, the programmer would have to manually replace the strings in the test code, increasing the work required.

A similar issue occurs if you want to change the behaviour of a field in Person. With the original approach, as most of the strings are located in CommandTestUtil.java, it is relatively simple to update the tests. The other approach would require manually changing every hardcoded name string.

There are many questionable design choices in the AB3 code, and I believe that it's intended as part of the learning experience.

[E0735389]

While working on the code, I realize another disadvantage of hard coding everything: The error messages tend to be very repetitive.

Nevertheless, in my opinion, hard coding short strings like the prefix a/ isn't really a problem.

[PallonCX]

Just to share some similar feeling on the complicated tests:

For me, I was really overwhelmed by how the various components of the AB3 main code worked at first, and it took me a few weeks to get used to it. After I wrote my first code increment, I realized that I also needed to write test cases for the new stuff I had added. I found out that the test functions were formed by complicated various components as well, but I couldn't afford to spend another few weeks understanding everything in tests just because I wanted to make the code increment.

I believe it would be beneficial if a better guide were given to students to get used to the codebase structure and understand why every class is necessary. It also happened a few times that I made some changes in the code, but unexpected errors occurred just because I wasn't familiar with the other part of the code that used the code that I edited.