Deal with end-of-lines in AbstractQuickfixTest independently from OS

[LorenzoBettini]

The AbstractQuickfixTest does not allow to easily customize comparison of strings in the presence of Windows end-of-line characters like "\r". That's especially the case when using new Java text blocks (which never contain \r) or in some cases where the quickfix might leave some spurious \r after the applications. Note that using the trick (used in Xtext tests itself):

    @Override
    public void testQuickfixesOn(CharSequence content, String issueCode, Quickfix... quickfixes) {
        super.testQuickfixesOn(Strings.toUnixLineSeparator(content), issueCode, quickfixes);
    }

does not help because of the above-mentioned spurious \r and Java text blocks.

A "drastic" way to solve the problem would be to always remove \r in org.eclipse.xtext.ui.testing.AbstractQuickfixTest.assertIssueResolutionResult(String, IssueResolution, String), e.g., something like

    protected void assertIssueResolutionResult(String expectedResult, IssueResolution actualIssueResolution, String originalText) {
        /*
         * Manually create an IModificationContext with an XtextDocument and call the
         * apply method of the actualIssueResolution with that IModificationContext
         */
        IXtextDocument document = getDocument(originalText);
        TestModificationContext modificationContext = new TestModificationContext();
        modificationContext.setDocument(document);

        new IssueResolution(actualIssueResolution.getLabel(), //
                actualIssueResolution.getDescription(), //
                actualIssueResolution.getImage(), //
                modificationContext, //
                actualIssueResolution.getModification(), //
                actualIssueResolution.getRelevance()).apply();
        String actualResult = document.get();
        assertEquals(
                     Strings.toUnixLineSeparator(expectedResult),
                     Strings.toUnixLineSeparator(actualResult));
    }

After all, I don't see why one would want to deal with \r (from the text document of the editor point of view, nothing changes in that respect).

Note that currently you can't fix the problem by simply overriding assertIssueResolutionResult: you could override that method, but "TestModificationContext" is private so there's not much one can do; I personally deal with this problem by using a custom XtextDocument in the tests, but I'd like to provide a cleaner solution.

If always using \n instead of \r\n as suggested above scares because of possible breakage in existing tests, at least, I would allow developers to redefine the comparison part, e.g., assertIssueResolutionResult could delegate the equal assertion to a protected method that the developer can redefine.

I can provide a PR once we somehow agree on a possible solution.

[szarnekow]

Some pseudo-random thoughts:

The quickfix should be applied with the correct line ending as expected by the document. Given that we make a new document without calling IDocumentExtension4 / XtextDocument.setInitialLineDelimiter I'd think it might make sense to use that API in the test along with a normalized model text in AbstractQuickfixTest.getDocument(String)

Now the parsed resource and the document should be in agreement on the used line separator.

This would leave the expected result: That one should be normalized to the same line delimiter and checked against the text that is read from the document as is.

Going one step further I'd like to see assertIssueResolutionResult to run against both popular line delimiters: windows and unix.

What do you think about this approach @LorenzoBettini ?

[LorenzoBettini]

@szarnekow I'm afraid I lost you pretty soon... O:)

[szarnekow]

@szarnekow I'm afraid I lost you pretty soon... O:)

What do you mean?

[LorenzoBettini]

Sorry, wrong translation from Italian :D I mean: I don't understand what you propose from the very beginning.

[szarnekow]

In a nutshell: Instead of modifying the actual and the expectation wrt to line endings, I'd prefer to modify the input model and the expectation. The line ending of the input model should in any case drive the line endings used by the quickfixes. So altering the actual result should never be necessary.

[LorenzoBettini]

I think that Java text blocks (which always have \n in any OS) would still make it fail. In fact, in my tests, I pass a Java text block as input, and when the quickfix that modifies the EMF model is applied, in the result I get also \r in Windows (only in the parts corresponding to the serialized parts of the modified model).

However, if you want, I can try your suggestion in Windows, but I'm not sure what changes I should do.

[szarnekow]

However, if you want, I can try your suggestion in Windows, but I'm not sure what changes I should do.

My idea and goal was to tun all tests as if it was Windows or Unix - independently from the current OS.

Step by step I'd say this is necessary:

1. Overload assertIssueResolutionResult and allow to pass additional line delimiter explicitly
2. Call the new method with \n and \r\n
3. assertIssueResolutionResult would modify the the original text as well as the expectation to replaceAll("\\R", givenLineDelimiter)
4. The XtextDocument has an initalLineDelimiter property. Set it to the given line delimiter
5. Apply the quickfix and check the resulting document content against the patched expected content.

[Bananeweizen]

I suggest to add AssertJ to your test dependencies. If I got your problem right, your code would become as simple as assertThat(actualQuickFix).isEqualToNormalizingNewlines(expectedQuickFix). You have all kinds of similar methods for removing or normalizing whitespace, unicode, line endings etc. See https://www.javadoc.io/doc/org.assertj/assertj-core/latest/org/assertj/core/api/StringAssert.html Assertions are generally way more easy to read and way more easy to write without additional helper methods in AssertJ than in plain JUnit.

[imhotep82]

I think the assertion would need to replace all line delimiters, compare the results disregarding all whitespace and on failure throw a ComparisonFailure with the two original values.

[LorenzoBettini]

However, if you want, I can try your suggestion in Windows, but I'm not sure what changes I should do.

My idea and goal was to tun all tests as if it was Windows or Unix - independently from the current OS.

Step by step I'd say this is necessary:

1. Overload assertIssueResolutionResult and allow to pass additional line delimiter explicitly

2. Call the new method with `\n` and `\r\n`

3. assertIssueResolutionResult would modify the the original text as well as the expectation to `replaceAll("\\R", givenLineDelimiter)`

4. The XtextDocument has an initalLineDelimiter property. Set it to the given line delimiter

5. Apply the quickfix and check the resulting document content against the patched expected content.

@szarnekow If I understand correctly, you suggest something like that:

    protected void assertIssueResolutionResult(String expectedResult, IssueResolution actualIssueResolution, String originalText) {
        assertIssueResolutionResult(expectedResult, actualIssueResolution, originalText, "\n");
        assertIssueResolutionResult(expectedResult, actualIssueResolution, originalText, "\r\n");
    }

    /**
     * @since 2.34
     */
    protected void assertIssueResolutionResult(String expectedResult, IssueResolution actualIssueResolution, String originalText, String lineDelimiter) {
        expectedResult = expectedResult.replaceAll("\\R", lineDelimiter);
        originalText = originalText.replaceAll("\\R", lineDelimiter);
        /*
         * Manually create an IModificationContext with an XtextDocument and call the
         * apply method of the actualIssueResolution with that IModificationContext
         */
        IXtextDocument document = getDocument(originalText, lineDelimiter);
        TestModificationContext modificationContext = new TestModificationContext();
        modificationContext.setDocument(document);

        new IssueResolution(actualIssueResolution.getLabel(), //
                actualIssueResolution.getDescription(), //
                actualIssueResolution.getImage(), //
                modificationContext, //
                actualIssueResolution.getModification(), //
                actualIssueResolution.getRelevance()).apply();
        String actualResult = document.get();
        assertEquals(expectedResult, actualResult);
    }

    /**
     * @since 2.34
     */
    protected XtextDocument getDocument(String model, String initialLineDelimiter) {
        XtextResource xtextResource = getXtextResource(model);
        XtextDocument document = injector.getInstance(XtextDocument.class);
        document.setInitialLineDelimiter(initialLineDelimiter);
        document.set(model);
        document.setInput(xtextResource);
        return document;
    }

is that right?

With this modification, if I remove the two overridden methods from this test:

@RunWith(XtextRunner.class)
@InjectWith(XtextUiInjectorProvider.class)
public class XtextGrammarQuickfixTest extends AbstractQuickfixTest {

    @Override
    protected void assertIssueResolutionResult(String expectedResult, IssueResolution actualIssueResolution, String originalText) {
        super.assertIssueResolutionResult(Strings.toPlatformLineSeparator(expectedResult), actualIssueResolution, Strings.toPlatformLineSeparator(originalText));
    }

    @Override
    public void testQuickfixesOn(CharSequence content, String issueCode, Quickfix... quickfixes) {
        super.testQuickfixesOn(Strings.toPlatformLineSeparator(content), issueCode, quickfixes);
    }
...

there are several failures

[LorenzoBettini]

The same holds for QuickfixTest in the Domainmodel example: a few failures with this modification

[LorenzoBettini]

@szarnekow as you can see from my last comment/attempt, it doesn't seem to work: there are spurious eols anyway. Could we go back to my initial proposal allowing customizing the behavior of verification?