[#3007] Content Assistant test infrastructure improvements. #3024

miklossy · 2024-04-30T13:02:45Z

Extend the Content Assistant test infrastructure to test proposals from several resources: modify the AbstractContentAssistTest getResourceFor method to ensure that the test file points to a file in the test project.
Uplift the testContentAssistant method from the project example classes into the AbstractContentAssistTest and rename it to assertContentAssistant.
Add the events_from_another_file() test case to the StatemachineContentAssistTest test cases to demonstrate how to test the content assistant with several resources.
Also add the ContentAssistWithSeveralResourcesTest test cases that are continuously executed by the CI build.

miklossy · 2024-04-30T13:03:27Z

This PR is based on the discussion between @LorenzoBettini and @cdietrich on the Xtext forum:
https://www.eclipse.org/forums/index.php/t/243876/

LorenzoBettini · 2024-04-30T14:23:59Z

org.eclipse.xtext.ui.testing/src/org/eclipse/xtext/ui/testing/AbstractContentAssistTest.java

+			 * A different resource with the URI 'platform:/resource/contentAssistTest/Test....' was already registered.
+			 */
+			Resource resource = resourceSet.getResource(resourceUri, false);
+			if (resource == null) {


But if the resource is not null, i.e., it exists, the passed input stream is not used to populate it, is it?

Yes. Exactly.

So I still don't understand... I was expecting the specified input stream to be taken into consideration and in case replace the current resource... maybe I'm still missing something

@LorenzoBettini : The method getResourceFor(InputStream stream) will be called twice within each test case execution, in both cases the passed stream is created from the currentModelToParse variable. So basically it does not matter if we use the first call and ingore the second call or if we create the resource on the first call and overwrite it with the same content in the second call.

I modified the PR so that in the second call the current resource content will be replaced, as requested.

@miklossy but the question now is why is it called twice?

It is a good question, but currently I am not sure if we could fix that without any breaking change. Let's analyse it in a follow up issue :-)

It is a good question, but currently I am not sure if we could fix that without any breaking change. Let's analyse it in a follow up issue :-)

OK, if it's always been like that :)

LorenzoBettini · 2024-04-30T14:25:55Z

org.eclipse.xtext.ui.testing/src/org/eclipse/xtext/ui/testing/AbstractContentAssistTest.java

+	 */
+	protected IFile createDslFile(String fileName, String fileExtension, CharSequence content) {
+		try {
+			return IResourcesSetupUtil.createFile(javaProject.getElementName(), fileName, fileExtension, content.toString());


it would be useful to specify also the path of the file, e.g., into the "src" directory. Does this allow that? If so, maybe it should be specified in the documentation.

To specity the worskpace relative path, the IResourcesSetupUtil#createFile(String, String) method can be used. I added this hint to the documentation.

So, I don't understand the benefit of this new method... again, I'm probably missing something, but why not simply consider the path in the first place?

This createDslFile method is basically a shorter way of calling the IResourcesSetupUtil.createFile method. The user can call this method if he/she wants to pass less parameter and does not want to bother with the exception handling. But if that does not fullfill his/her usecase, than he/she can call directly the IResourcesSetupUtil.createFile method instead.

OK, but in any case, can the passed fileName consist of a path? I think that's important. I mean, it's important to be able to specify a path, otherwise that method is unusable for Xbase languages where files must be in source folders.

@LorenzoBettini : I extended the javadoc as follows:

@param fileName * The name of the file (without the file extension). * It is also possible to use project relative path something like: <code>src/foo/bar/Test</code> * To be able to specify the workspace relative path, use {@link IResourcesSetupUtil#createFile(String, String)} instead. *

LorenzoBettini · 2024-04-30T14:31:37Z

org.eclipse.xtext.ui.testing/src/org/eclipse/xtext/ui/testing/AbstractContentAssistTest.java

+	 * 
+	 * @since 2.35
+	 */
+	protected void assertContentAssistant(CharSequence text, List<String> expectedProposals) {


Since we're creating a new API, could we take the chance to allow for expected proposals to be specified also as a string where proposals are separated by a new line? This would allow for easier TDD or test adjustments: it's easier to specify or update a string than to create a list of proposals or update it. Turning the string into a list or array of strings is easy with Java streams. @szarnekow wdyt?

LorenzoBettini · 2024-04-30T14:33:49Z

...i.tests/src/org/eclipse/xtext/example/fowlerdsl/ui/tests/StatemachineContentAssistTest.xtend

 	@Test def empty() throws Exception {
 		'''
 			«c»
-		'''.testContentAssistant(#[
+		'''.assertContentAssistant(#[


For example, with the additional API proposed above, here you could do

''' commands events resetEvents '''

Good idea @LorenzoBettini ! I updated the PR so that it is also possible to specify the expected proposals as one CharSequence containing the expected proposals separated by new lines.

cdietrich · 2024-05-02T04:15:50Z

org.eclipse.xtext.ui.testing/src/org/eclipse/xtext/ui/testing/AbstractContentAssistTest.java

-			Resource resource = resourceSet.createResource(resourceUri);
-			resource.load(stream, null);
+			String projectFullPath = javaProject.getProject().getFullPath().toString();
+			URI resourceUri = URI.createPlatformResourceURI(projectFullPath + "/" + "Test." + fileExtensionProvider.getPrimaryFileExtension(), true);


am not sure. dont in java projects. model files need to reside in source folder?

Exactly! That's why I was suggesting below to allow for relative path specification.

@miklossy I still don't understand this part... see my comment and @cdietrich's one

from my pov this is still open

@cdietrich relative paths are allowed, see the other answers. What did you mean?

i mean the default. unfortunately i dont have cappa to try it

Ok, now maybe I see: the file on which the content assistant is triggered! That's always the same and cannot be created in a specific folder. Is that right @miklossy ? If it's so, then we should allow for some customization? Maybe a variable that can be changed? And maybe the default should be "src/Test"?

@LorenzoBettini : good idea! I have just modified the PR as requested by introducing the following method:

/** * Returns the project relative path where the files should be created. * The default implementation creates them directly in the src folder. * Clients may override. * * @since 2.35 */ protected String getProjectRelativePath() { return "src"; }

@miklossy Thanks!
@cdietrich I think this also fixes your last issue.

LorenzoBettini · 2024-05-04T08:10:06Z

org.eclipse.xtext.ui.testing/src/org/eclipse/xtext/ui/testing/AbstractContentAssistTest.java

-			Resource resource = resourceSet.createResource(resourceUri);
-			resource.load(stream, null);
+			String projectFullPath = javaProject.getProject().getFullPath().toString();
+			URI resourceUri = URI.createPlatformResourceURI(projectFullPath + "/" + "Test." + fileExtensionProvider.getPrimaryFileExtension(), true);


@miklossy I still don't understand this part... see my comment and @cdietrich's one

LorenzoBettini · 2024-05-04T08:12:05Z

org.eclipse.xtext.ui.testing/src/org/eclipse/xtext/ui/testing/AbstractContentAssistTest.java

+			 * A different resource with the URI 'platform:/resource/contentAssistTest/Test....' was already registered.
+			 */
+			Resource resource = resourceSet.getResource(resourceUri, false);
+			if (resource == null) {


So I still don't understand... I was expecting the specified input stream to be taken into consideration and in case replace the current resource... maybe I'm still missing something

LorenzoBettini · 2024-05-04T08:12:51Z

org.eclipse.xtext.ui.testing/src/org/eclipse/xtext/ui/testing/AbstractContentAssistTest.java

+	 */
+	protected IFile createDslFile(String fileName, String fileExtension, CharSequence content) {
+		try {
+			return IResourcesSetupUtil.createFile(javaProject.getElementName(), fileName, fileExtension, content.toString());


So, I don't understand the benefit of this new method... again, I'm probably missing something, but why not simply consider the path in the first place?

LorenzoBettini · 2024-05-04T08:16:25Z

org.eclipse.xtext.ui.testing/src/org/eclipse/xtext/ui/testing/AbstractContentAssistTest.java

+	 * @since 2.35
+	 */
+	protected void assertContentAssistant(CharSequence text, CharSequence expectedProposals) {
+		assertContentAssistant(text, expectedProposals.toString().split(System.lineSeparator()), null, null);


Maybe I opened a Pandora's box... O:)
I'd like to avoid introducing further problems due to d**n line endings in Windows.
Your implementation works when using Xtend template strings but would fail when using Java text blocks or Xtend multi-line strings that are now (#3004) normalized w.r.t. line endings (only Unix EOL is generated).

I'd like this solution to be guarded against different EOLs...

A possible solution would be to first normalize the expectedProposals with only Unix EOL (we have a utility method Strings.toUnixLineSeparator) and then split it only according "\n".

I haven't tried that, but it should work.

@LorenzoBettini : I have just modified the PR as requested:

protected void assertContentAssistant(CharSequence text, CharSequence expectedProposals) { String[] expectedProposalsArray = Strings.toUnixLineSeparator(expectedProposals.toString()).split("\n"); assertContentAssistant(text, expectedProposalsArray, null, null); } protected void assertContentAssistant(CharSequence text, CharSequence expectedProposals, String proposalToApply, String expectedContent) { String[] expectedProposalsArray = Strings.toUnixLineSeparator(expectedProposals.toString()).split("\n"); assertContentAssistant(text, expectedProposalsArray, proposalToApply, expectedContent); }

@miklossy Thanks! I feel safer now ;)

Just in case, can't the first method be implemented in terms of the second one, by simply passing text, expectedProposals, null, null?

I modified the PR.

LorenzoBettini · 2024-05-04T08:18:24Z

org.eclipse.xtext.ui.testing/src/org/eclipse/xtext/ui/testing/AbstractContentAssistTest.java

+	 * 
+	 * @since 2.35
+	 */
+	protected void assertContentAssistant(CharSequence text, String[] expectedProposals) {


So you removed the version with List?

That could have been useful in Xtend with #[ ... ]. Or would it work anyway thanks to conversion?

Moreover, if we want to use arrays, why not using String... expectedProposals, which would not require the creation of an array when calling this method? Would it conflict with the one accepting a String?

LorenzoBettini · 2024-05-04T08:18:53Z

org.eclipse.xtext.ui.testing/src/org/eclipse/xtext/ui/testing/AbstractContentAssistTest.java

+	 * @since 2.35
+	 */
+	protected void assertContentAssistant(CharSequence text, CharSequence expectedProposals, String proposalToApply, String expectedContent) {
+		assertContentAssistant(text, expectedProposals.toString().split(System.lineSeparator()), proposalToApply, expectedContent);


As above, concerning EOL in the passed string.

LorenzoBettini · 2024-05-04T08:19:45Z

...s/org/eclipse/xtext/ui/tests/editor/contentassist/ContentAssistWithSeveralResourcesTest.java

+	@Test public void test_with_two_resources() {
+		createDslFile("types", "type A extends A");
+		IResourcesSetupUtil.waitForBuild();
+		assertContentAssistant("import \"types.importuriuitestlanguage\" type B extends " + c, new String[]{"A", "B"});


Here, I would call it with "A\nB" to make sure my proposal above about EOLs works as expected.

I have just modified the PR as requested.

LorenzoBettini · 2024-05-04T08:20:52Z

...ainmodel.ui.tests/src/org/eclipse/xtext/example/domainmodel/ui/tests/ContentAssistTest.xtend

@@ -56,7 +52,7 @@ class ContentAssistTest extends AbstractContentAssistTest {
 			entity E {
 				«c»
 			}
-		'''.testContentAssistant(#[
+		'''.assertContentAssistant(#[


OK, so, concerning my comment above about String[] instead of List, this seems to work out of the box with Xtend, right?

Yes, Xtend automatically converts the actual parameter from array to list (and vice versa) if the formal parameter is a list/array.

LorenzoBettini · 2024-05-10T10:40:33Z

Looks like GitHub Actions is having bad days lately... the ubuntu build failed with a timeout.

Let's wait for the build to finish and then we re-run the failing jobs, just in case.

LorenzoBettini

@miklossy I'm fine with the PR, but there's still my question pending about the path in the passed fileName

miklossy · 2024-05-10T12:53:26Z

@miklossy I'm fine with the PR, but there's still my question pending about the path in the passed fileName

It is not possible to use absolute path there, but you can use relative path, something like:
src/foo/bar/testfile

LorenzoBettini · 2024-05-10T13:33:00Z

@miklossy I'm fine with the PR, but there's still my question pending about the path in the passed fileName

It is not possible to use absolute path there, but you can use relative path, something like: src/foo/bar/testfile

No no, I meant project relative path.
I see you have updated the documentation, so to me that's fine! :)

LorenzoBettini

Thanks!

@cdietrich wdyt?

- Extend the Content Assistant test infrastructure to test proposals from several resources: modify the AbstractContentAssistTest getResourceFor method to ensure that the test file points to a file in the test project. - Uplift the testContentAssistant method from the project example classes into the AbstractContentAssistTest and rename it to assertContentAssistant. - Add the events_from_another_file() test case to the StatemachineContentAssistTest test cases to demonstrate how to test the content assistant with several resources. - Also add the ContentAssistWithSeveralResourcesTest test cases that are continuously executed by the CI build. Closes #3007 Signed-off-by: miklossy <miklossy@itemis.de>

miklossy · 2024-05-13T07:49:53Z

@LorenzoBettini : do you see why the build is red?

LorenzoBettini · 2024-05-13T08:02:11Z

@LorenzoBettini : do you see why the build is red?

macOS on GitHub Actions is completely unreliable and slow lately.
I'll try to re-run. If it still fails, I'll still merge this.

miklossy · 2024-05-13T15:38:43Z

Hmm.. The MacOS build is still red, but the windows and the ubuntu builds are green. Could we merge it, @LorenzoBettini ?

LorenzoBettini · 2024-05-13T16:38:14Z

Done! Thanks @miklossy .
Do you think it's worth to say something in the release notes?

miklossy · 2024-05-13T16:42:57Z

Yes, see eclipse/xtext-website#43 :-)

Thanks @LorenzoBettini for your entire support on this PR!

miklossy added this to the Release_2.35 milestone Apr 30, 2024

miklossy requested review from cdietrich and LorenzoBettini April 30, 2024 13:02

LorenzoBettini reviewed Apr 30, 2024

View reviewed changes

cdietrich reviewed May 2, 2024

View reviewed changes

miklossy force-pushed the miklossy_issue_3007 branch from 79797f9 to 33665f1 Compare May 3, 2024 13:27

LorenzoBettini requested changes May 4, 2024

View reviewed changes

miklossy force-pushed the miklossy_issue_3007 branch 2 times, most recently from fe3f940 to 75049df Compare May 10, 2024 09:36

LorenzoBettini reviewed May 10, 2024

View reviewed changes

miklossy force-pushed the miklossy_issue_3007 branch 2 times, most recently from ef2e617 to 4f31128 Compare May 10, 2024 13:01

LorenzoBettini approved these changes May 10, 2024

View reviewed changes

cdietrich approved these changes May 11, 2024

View reviewed changes

miklossy force-pushed the miklossy_issue_3007 branch from 4f31128 to ddef354 Compare May 13, 2024 06:16

LorenzoBettini merged commit 9ad9582 into main May 13, 2024
10 checks passed

LorenzoBettini deleted the miklossy_issue_3007 branch May 13, 2024 16:37

LorenzoBettini mentioned this pull request May 21, 2024

Regression in AbstractContentAssistTest breaking customizations #3051

Closed

[#3007] Content Assistant test infrastructure improvements. #3024

[#3007] Content Assistant test infrastructure improvements. #3024

Conversation

miklossy commented Apr 30, 2024

miklossy commented Apr 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LorenzoBettini commented May 10, 2024

LorenzoBettini left a comment

Choose a reason for hiding this comment

miklossy commented May 10, 2024

LorenzoBettini commented May 10, 2024

LorenzoBettini left a comment

Choose a reason for hiding this comment

miklossy commented May 13, 2024

LorenzoBettini commented May 13, 2024

miklossy commented May 13, 2024

LorenzoBettini commented May 13, 2024

miklossy commented May 13, 2024