Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change tests so that folder name and project name are distinct #343

Closed
wants to merge 1 commit into from
Closed

Conversation

xorye
Copy link

@xorye xorye commented Jul 8, 2020

Currently, the tests introduced in #330 succeed when the project labels command retrieves the project name as both project.getName() or project.getDescription().getName() here: https://github.com/xorye/quarkus-ls/blob/4a1dae985db0ef0cfa9f7f8bac82e54bb6b878fd/microprofile.jdt/com.redhat.microprofile.jdt.core/src/main/java/com/redhat/microprofile/jdt/core/ProjectLabelManager.java#L83

This PR ensures that tests pass if project.getDescription().getName() is used, and fail if project.getName() is used.

Signed-off-by: David Kwon <dakwon@redhat.com>
@xorye
Copy link
Author

xorye commented Jul 8, 2020

One moment please, I will explain why this PR removes some tests.

@@ -187,7 +181,7 @@ public void run(IProgressMonitor monitor) throws CoreException {
// property:
// deployment-artifact=io.quarkus\:quarkus-hibernate-orm-deployment\:0.21.1

IJavaProject javaProject = JavaModelManager.getJavaModelManager().getJavaModel().getJavaProject(description.getName());
IJavaProject javaProject = JavaModelManager.getJavaModelManager().getJavaModel().getJavaProject(projectName);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After a discussion with @fbricon, we can stick with Maven tests for now. When loading Maven projects (ie, from VS Code), the IResource path is P/folderName, therefore we should use projectName (which is the folder name) here instead of description.getName().

This essentially reverts a change that #330 did.

@Test
public void projectNameSameFolderName() throws Exception {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test tested the case where you have two folders in your workspace with the same folder name, but different project names specified in .project.

I removed this case because for maven projects, since this the IResource path is their folder name, if two maven projects have the same folder name, they would share the same IProject object.

In other words, this test would try to load two projects, but only one project would be created.

Also, it seems that in Eclipse, by default, you cannot have two maven project folders with the same name in the package explorer even though the two projects have different names in .project.

List<ProjectLabelInfoEntry> projectLabelEntries = ProjectLabelManager.getInstance().getProjectLabelInfo();
assertName(projectLabelEntries, quarkusGradle, "quarkus-gradle-project");
assertName(projectLabelEntries, gradle, "empty-gradle-project");
assertName(projectLabelEntries, renamedGradle, "my-gradle-project");
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When loading Gradle projects (ie, from VS Code), if the folder name is folder-name and the project name in .project is new-name, unlike loading Maven projects, I've observed that the IResource path is P/new-name instead of P/folder-name.

Since Maven projects are being focussed on, for now, I removed the Gradle test case where folder name != project name.

Copy link
Author

@xorye xorye Jul 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This removed test case will cause a NPE since this PR loads projects using folder name: JavaModelManager.getJavaModelManager().getJavaModel().getJavaProject({folder name here}); in BasePropertiesManagerTest.java.

@xorye xorye marked this pull request as draft July 8, 2020 20:07
@xorye xorye closed this Jul 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants