eclipse-xtext / eclipse-xtext/xtext

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

Open
#2,483 12 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Java
Stars
831
Forks
330
Avg merge
3d 7h
Merged PRs (30d)
12

Description

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.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in org.eclipse.xtext.ui.testing.AbstractQuickfixTest, especially assertIssueResolutionResult(String, IssueResolution, String), and inspect how TestModificationContext applies resolutions. Determine whether line-ending normalization or a protected comparison hook best fits the existing API; done means quickfix tests can compare results reliably across Windows line endings and Java text blocks without breaking existing tests.

Written by the indexing model from the issue text.

Assessment

Tech stack
java
Domain
testing
Issue type
Feature
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
38/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.