Unit testing logic is broken

[prestoncrawford]

Expected Behaviour

The logic in the unit testing section is broken. The new unit tests are written such that if ANY field is empty, the assertions pass. This isn't the behavior you want, however, for tests that are for specific fields. If you modify the ctx.currentResource() call for testIsEmpty_WithoutName, for example, and set it to a resource of /content/without-occupations this should fail, because there is a name in that context. In fact, this test will pass, because the check is for isEmpty and isEmpty will return to true because one of the fields is empty. In this case the occupations field.

Actual Behaviour

I would propose modifying the isEmpty method to be more clear and adding discrete methods to test the other fields. Otherwise when someone pokes around this they may not understand why tests are passing when they shouldn't be and visa versa.

Reproduce Scenario (including but not limited to)

In the test testIsEmpty_WithoutName() set the resource as follows`

ctx.currentResource("/content/without-occupations");

Note that this will pass, meaning it's "empty", even though the context does have a name field.

Steps to Reproduce

Propose modifying the tests and the code examples on the site.

The code examples could read something like this.

    void testIsEmpty() {
        ctx.currentResource("/content/empty");
        Byline byline = ctx.request().adaptTo(Byline.class);

        assertTrue(byline.isAnyFieldEmpty());
    }

    @Test
    public void testIsEmpty_WithoutName() {
        ctx.currentResource("/content/without-name");

        Byline byline = ctx.request().adaptTo(Byline.class);

        assertTrue(byline.isNameEmpty());
    }

    @Test
    public void testIsEmpty_WithoutOccupations() {
        ctx.currentResource("/content/without-occupations");

        Byline byline = ctx.request().adaptTo(Byline.class);

        assertTrue(byline.isOccupationsEmpty());
    }

    @Test
    public void testIsEmpty_WithoutImage() {
        ctx.currentResource("/content/byline");

        lenient().when(modelFactory.getModelFromWrappedRequest(eq(ctx.request()),
                any(Resource.class),
                eq(Image.class))).thenReturn(null);

        Byline byline = ctx.request().adaptTo(Byline.class);

        assertTrue(byline.isImageEmpty());
    }```

[prestoncrawford]

Created a PR for this if I'm correct and someone is interested. This will, of course, require changes to other branches as well, but I stopped short in case those should be approached differently.

https://github.com/adobe/aem-guides-wknd/pull/408

[prestoncrawford]

I saw that my PR was approved and merged. Apologies for any confusion caused the typo where I mentioned issue 406.

Anyway, please note that documentation will have to be updated as well here. It's unclear if there's anywhere to update that in a repo, so I'm just leaving a note here.

Also, similar changes will need to be made to the branches unit-testing-solution as well as main. It's not clear what your branching strategy is, though. If you just merge those down and keep the branches alive or what.

[davidjgonzalez]

@prestoncrawford i see where youre going w/ this ... but im struggling to see how breaking these checks out into testable functions gets you anything more at the end of the day.

For example:

You ca test the discrete isXEmpty() methods, but ultimately the thing we care about is testing isEmpty(), since that's what our HTL uses (Just because the discrete isXEmpty*() functions pass, doesnt mean there isn't a bug in isEmpty()).

Ultimately we need to check that isEmpty() returns false when a) name is empty b) occupations is empty c) image is empty, and that's what the test data is setup to test:

• testIsEmpty_WithoutName -> tests having only occupation and image
• testIsEmpty_WithoutOccupations -> tests having only name and image
• testIsEmpty_WithoutImage -> tests having only name and occupations
• testIsEmpty_WithoutImageSrc -> having only a name and occupations (and non-null image object)

You can of course muck w/ any tests input, but at that point you're writing a bad test and should fix the input. Ultimately tests are intended to allow you to change the code being tested (BylineImpl.java) without breaking existing behavior; since we have 100% code coverage as well - we can be confident that all pathing is covered by our tests (and their respective inputs).

Or maybe asked another way ... if we break the checks out into discrete methods isNameEmpty, isOccupationsEmpty and isImageEmpty - how do you propose writing a test for isEmpty() that looks like:

    public boolean isEmpty() {
        // If any of the required fields are empty, the component is considered empty
        return isNameEmpty() || isOccupationsEmpty() || isImageEmpty();
    }

...that isn't susceptible to returning a false positive if you mess with your input data?

[prestoncrawford]

@davidjgonzalez - I'm only proposing a fix to make the tutorial more clear. I understand the intent of isEmpty as it concerns the HTL.

Sorry if I misunderstood the point of the unit tests. It just struck me that if you're working through this tutorial here, tests like testIsEmpty_WithoutOccupations don't actually work as expected. You can use the test data that leaves out names, but includes occupations and you will get a false positive on the test. I would think this would be confusing to people running through the tutorial. It was to me.

It caused me to recheck my test data, which is how I found this.

I still think there should be a check of isEmpty. Just that the tests for testIsEmpty_WithoutOccupations, testIsEmpty_WithoutName, etc. should be meaningful tests.

[davidjgonzalez]

@prestoncrawford perhaps the various testisEmpty_withoutXXX should include a comments making it clear:

• What each test includes as part of the test criteria/input
• What each test excludes as part of the test criteria (its in the method name, but could call it out more clearly in a comment).

[davidjgonzalez]

Updated the tutorial with clarifying text