WIP: Add a test about merged cells that should fail

[sschuberth]

DO NOT MERGE YET!

This is a follow-up to issue #14. To start with, I'd like to understand why this test passes although I'd expect it to fail.

[tobyweston]

Doh, I completely misread your comments, you'd expect this to fail when it passes. Gotcha, will take another pass. Ignore me.

[tobyweston]

I changed this to use getWorkbook as they seem equivalent with the same result (i.e. a passing test)

[sschuberth]

Yeah, that probably does not make a difference. I just wanted to stick as close as possible to my original code which uses WorkbookFactory.create() to rule out any unwanted side effects.

[tobyweston]

Not clear why this is new, it seems to do the same as getWorkbook unless I'm missing something? I think I prefer the getWorkbook as it ensures the stream is closed and (for some reason I've long since forgotten) deals with exceptions in a certain way.

[sschuberth]

Same reason as above.

[tobyweston]

and I assume you really want something like

assertThat(workbook, is(not(sameWorkbook(getWorkbook("emptySheet.xlsx")))));

and maybe @Ignore the test.

Interestingly, the output when this test fails is terrible.

Expected: is not entire workbook to be equal
     but: was <Name: /xl/workbook.xml - Content Type: application/vnd.openxmlformats-officedocument.spreadsheetml.sheet.main+xml>
Expected :is not entire workbook to be equal
     
Actual   :<Name: /xl/workbook.xml - Content Type: application/vnd.openxmlformats-officedocument.spreadsheetml.sheet.main+xml>

The matcher stuff is all about making this output easy to read, so we should do something about that also.

[tobyweston]

Be nice to add an assertion here to ensure/demonstrate that the addMergedRegion did what it's supposed to. I was a bit confused how we should expect the underlying sheet to have changed just reading it cold.

[tobyweston]

I think I'd just never considered merged regions before. This comes with caveats as I haven't spent too long looking but if you change the test to include:

assertThat(workbook, is(not(sameWorkbook(getWorkbook("emptySheet.xlsx")))));

so it fails, you can add update the SheetsMatcher with something (crude) like this:

    protected boolean matchesSafely(Workbook actual, Description mismatch) {
        for (Sheet expectedSheet : sheetsOf(expected)) {
            Sheet actualSheet = actual.getSheet(expectedSheet.getSheetName());

            if (actualSheet.getMergedRegions() != expectedSheet.getMergedRegions())
                return false;
            
            if (!hasSameNumberOfRowAs(expectedSheet).matchesSafely(actualSheet, mismatch))
                return false;

            if (!hasSameRowsAs(expectedSheet).matchesSafely(actualSheet, mismatch))
                return false;
        }
        return true;
    }

..and the test will pass.

If you agree with my assessment (that we need to add in a merged cell check, I propose we replace the crude addition above with a new Matcher, something like MergedRegionMatcher extending TypeSafeDiagnosingMatcher<Sheet> as per modus operandi, then wire it in the to sheet matcher.

That would imply associated tests, testing the matcher and you'd have to have a think about what makes sense to "match" on in that matcher, i.e. is it enough to just compare the merged region array (I can't remember how Java does array comparisons either, do you need to enumerate or use equals vs ==?).

Also those tests should demonstrate the output description when they don't match. It would be awesome to say something like "expected merged range 1 to 5 but found no merged ranges".

The icing on the cake would be an improvement to the current failing test output which I mentioned in the PR. This is just the toString of a Workbook at the moment but we could probably do something better.

[tobyweston]

I'm prob going to close this PR as I've merged your changes into the merged_region branch. As and when it's merged to master, your commits will be included.

If you'd like to carry on, might be better to checkout the branch and play with that.

[sschuberth]

I'm fine with closing this PR, it's only a draft anyway, and I'll not have the time to debug the issue further in the foreseeable future, sorry 😢