Unit testing logic is broken about aem-guides-wknd HOT 6 CLOSED

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.

#408

from aem-guides-wknd.

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.

from aem-guides-wknd.

@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?

from aem-guides-wknd.

@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.

from aem-guides-wknd.

@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).

from aem-guides-wknd.

Updated the tutorial with clarifying text

from aem-guides-wknd.