-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Rework top test menu items #4980
Conversation
https://issues.jboss.org/browse/CHE-227 Signed-off-by: David Festal <dfestal@redhat.com>
Can one of the admins verify this patch? |
@l0rd Could you review this one ? |
ci-build |
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/2555/ |
@vparfonov @slemeur is there anything more I should do in order to have this PR merged ? |
Need QA confirmation |
e.getPresentation().setVisible(true); | ||
|
||
String projectType = project.getType(); | ||
boolean enable = "maven".equals(projectType); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we need here checking for "maven" what about "javac" project?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vparfonov : because for the moment the Java (JUnit / TestNG) testing feature is only supported on maven Java projects. that was from the beginning of this feature afaik. But sure as soon as this limitation is dropped, we'll be able to remove this test.
Basically ok for me except adding checking on "maven" project type. Why we need it? |
@davidfestal |
Well, when reworking the testing menu, I tried to fix inconsistencies in the overall behavior, and thus had to understand / rethink the menu activation rules. And it seemed to me that, when you're explicitly requesting tests on a Java file, the aim is not to start a test whose input is a specific unrelated XML file. So when I disabled the |
@musienko-maxim are you OK with my comment and is it possible to merge now ? |
For me ok. |
@vparfonov ? can we merge |
ci-build |
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/2709/ |
What does this PR do?
This PR reworks the behaviour of the top menu
Run -> Test
menu items.Until now, these menus items were based on the Java Editor only. So when no Java Editor had the focus, the menu items were disabled, even with a Java source file selected in File Explorer. This was leading to an inconsistent behaviour between the Project Explorer
Test
contextual menu and theRun -> Test
top menu item.With this PR, the proposed test menu items are based on the current resource, either from the project explorer or the currently opened editor, according to which one has the focus. This brings back consistency between both menus.
What issues does this PR fix or reference?
https://issues.jboss.org/browse/CHE-227
Changelog
Provide a consistent behaviour between the
Run -> Test
top menu item and theRun Test
project explorer contextual menu.Release Notes
Provide a consistent behaviour between the
Run -> Test
top menu item and theRun Test
project explorer contextual menu.Docs PR