Community
Participate
Working Groups
Build Identifier: Version: Indigo Release Build id: 20110615-0604 With around 150 unit test in a test suite, it is hard to find and re-execute / debug a specific test case by name. Many other Eclipse views have "Link with Editor" buttons that allow for a view to always select the item that is being edited. This may apply for the JUnit view, too. If the current selection in an Editor matches the displayed JUnit execution content, the test case could be selected and given focus in the JUnit view. (i.e. the Editor's cursor is in a method annotated with @Test and previously executed with JUnit) Alternatively, like the Outline view, the JUnit view could have a "Sort" button to sort all test cases alphabetically. These two features wouldn't be used very frequently, so they could be put in the "Advanced Section" dropdown Reproducible: Always Steps to Reproduce: See Details
Moving to JDT/UI (In reply to comment #0) > Build Identifier: Version: Indigo Release Build id: 20110615-0604 > > With around 150 unit test in a test suite, it is hard to find and re-execute / > debug a specific test case by name. You can select the method name in the editor, and right click and choose "Run As>Junit Test". To navigate from Junit view to editor, just double click on any test name in the Junit view.
Sorting is covered by bug 219466.
(In reply to comment #1) > Moving to JDT/UI > (In reply to comment #0) > > Build Identifier: Version: Indigo Release Build id: 20110615-0604 > > > > With around 150 unit test in a test suite, it is hard to find and re-execute / > > debug a specific test case by name. > You can select the method name in the editor, and right click and choose "Run > As>Junit Test". > To navigate from Junit view to editor, just double click on any test name in > the Junit view. Very nice, I wasn't aware of this! It's quite subtle though. When clicking anywhere else (even inside the method), the whole test suite is executed. But I can also right-click on the method in the Outline to execute that single test case, so that's a good workaround Thanks
I just noticed that the workarounds (run a test from the context menu when selecting the test method in the Editor / Outline) don't always work. I have a use case, where I have an abstract test class, declaring the actual test methods, and several concrete subclasses inheriting all the tests. In that setup, I can only run tests from the JUnit view...
Created attachment 235306 [details] Proposed patch Please find attached a patch proposal for this issue. Basically, this patch adds a toggle action to enable/disable 'Link with Editor' (disabled by default) - When 'Linking with editor' is enabled, an inner IPartListener2 is added to the active page, otherwise it is removed. - When the selection in the active Java Editor editor changes (eg: user switching to another CompilationUnitEditor or moving selection in another method of the same Test class), the selection in the TestViewer is updated. Also, this patch should not add too much overhead if the active Java Editor is not opened on a Test class or if the user only moves the selection within the same method. Please, let me know if this patch is fine or if I should make some changes to get it approved. Thanks.
We are very busy working on Java 8 support, but might find a bit of time for an initial review and give an initial feedback. Please stay tuned. Manju, please start an initial review once you have some free time.
Thanks for the patch. Applied your patch and tried the functionality. The expected behavior would be similar to the Outline view where, when a member is selected, the same should be highlighted in the editor. With the patch I can see that when the test method is selected in the editor, the JUnit view is updated accordingly, but when the test method is selected(single mouse click) in the JUnit view, then the method is not highlighted even when the test class is the currently active editor. Another expected behavior would be if the test class is opened but is not the currently active editor, then when user selects a test method in the JUnit view, the class file is set as the currently active editor and the test method highlighted. Will look in to the code once the above points are taken care off.
"Another expected behavior would be if the test class is opened but is not the currently active editor, then when user selects a test method in the JUnit view, the class file is set as the currently active editor and the test method highlighted." This isn't how other Link with editor features works - its a one-way street; selections in editor is reflected in the outline/junit view. Not the other way around - that requires a double click IMO.
(In reply to Max Rydahl Andersen from comment #8) > "Another expected behavior would be if the test class is opened but is not > the currently active editor, then when user selects a test method in the > JUnit view, the class file is set as the currently active editor and the > test method highlighted." > > This isn't how other Link with editor features works - its a one-way street; > selections in editor is reflected in the outline/junit view. Not the other > way around - that requires a double click IMO. No, that is not true.
doh - I stand corrected :/ Never noticed that when the editor is open for a file it will jump to it. Sorry for claiming otherwise.
Hello, Sorry for being late on this patch. I took Manju Mathew's comments into account and added the missing part, ie, when the user selects a test method in the TestViewer, the associated Java method is selected in the Java Editor. The Java Editor will also be activated (shown) if necessary, but not opened. Please, let me know if this new patch is fine or if you need some adjustments. Thanks.
Created attachment 239512 [details] Second patch with full implementation Added missing features, as asked by Manju Mathew.
1. The patch doesn't apply neatly in my WS, always pull changes from Git before creating a new patch. 2. After copying the changes manually I can see that, this implementation also do not meet the requirement mentioned in comment 7. 3. Went through the code and the implementation of #getEditorClassName() is not correct. The path separator is OS dependent and hence you cannot always expect "/" as the separator, use: sourcePath.removeFileExtension().toOSString().replace(File.separatorChar, '.') 4. As said earlier it should behave similar to Outline View. For instance open Outline View, Javadoc View and the Editor. Make sure "Link With Editor" feature in Javadoc View is turned on. Now if you select any method in Outline View, Java editor is updated and Javadoc View is also updated accordingly. This behavior is missing in the current patch with JUnit View. Have a look at different utility classes mentioned in JDTUIHelperClasses.java, many methods can come handy during development. Also follow the development guidelines mentioned in http://wiki.eclipse.org/JDT_UI/How_to_Contribute#Coding_Conventions
Hello Manju, I'm going to look why the patch did not apply natively on your WS. I'm pretty sure I did pull the latest changes, though. Can you give me more input on which behavior is still missing with regards to the requirements you mentionned in comment #7 ? I guess I missed something.. I'm also going to look at the other comments and modify my patch accordingly. Thanks. Best regards,
Manju, I can see merge issues when merging the patch with EGit. I prepared the patch from CLI, which might explain the problems. Sorry about that. Next patch will be prepared with EGit to avoid the troubles again. Best regards,
Manju, I can see what you meant in item 4: "4. As said earlier it should behave similar to Outline View. For instance open Outline View, Javadoc View and the Editor. Make sure "Link With Editor" feature in Javadoc View is turned on. Now if you select any method in Outline View, Java editor is updated and Javadoc View is also updated accordingly. This behavior is missing in the current patch with JUnit View." I'll take care of this one, too. Best regards,
Created attachment 239779 [details] New patch Hello, This is an (almost full) rework from the previous patch, and hopefully this time it implements all the features you requested in your previous comments. Besides, I created the patch from EGit so I hope you won't have any trouble merging it in your codebase. Please, let me know if it's OK this time ;-) Best regards, Xavier
I've been told that Gerrit is better et faster for Code reviews, so I pushed my patch over there, too: https://git.eclipse.org/r/#/c/21752/ if you want to use this tool to review and discuss about it. Thanks. Best regards, Xavier
Comment on attachment 239779 [details] New patch This time the patch almost worked. 1) After applying the patch there is a compilation error in TestCaseElement.java, org.eclipse.jdt.junit.core is using Java 1.4, hence you cannot use StringBuilder (available from Java 1.5 and above). Replace the usage with StringBuffer. 2) The sync works util you switch the editor. Consider you have a TestSync.java(execute the tests in this class) and A.java files opened in editor. I have JUnit view and Outline view opened. The sync works perfectly fine in both ways when TestSync.java is the active editor. Now activate/switch to A.java and now come back to TestSync.java. Click on a test method in Outline view, the method is highlighted in the editor, but JUnit view is not in sync now. Now to get it back to working i need to reset the "Link with Editor" action. 3) Yet to do a code review, will take that up only after the functionality is up and running.
Manju, Thanks for the reply. I'll take care about items #1 and #2 ASAP. Best regards, Xavier
Created attachment 239813 [details] Third patch with full implementation and bug fixes This patch replaces the previous one. - it fixes the compilation problem by using StringBuffer instead of StringBuilder. - it initializes the internal IPartListener2 of TestViewer at startup if the "Link with Editor" was active in the previous workbench session, resolving the problem where the link with editor would not work after selecting another editor.
Comment on attachment 239813 [details] Third patch with full implementation and bug fixes This patch was also disappointing. Some of the features that were working from your previous patch stopped working in this patch :-( We totally understand if some corner cases are missed, but you cannot post a patch without doing the bare minimal blackbox testing. Features not working at first glance: 1. If "Link with editor" is already switched on when the workbench is up, then selecting methods in Outline view does not highlight the method in the JUnit view. Re-enabling the "Link with editor" rectifies the issue. 2. Switch the editor and come back to the test case. Select any method in Outline view, again the sync is broken. Please do a through testing before you post another patch.
Manju, I am terribly sorry about those remaining bugs. In regards with what you reported, my testing was actually not good enough :-/ I'll fix them and submit another patch ASAP.
Created attachment 240197 [details] New attempt for the patch Here's a new patch which fixes the problems reported by Manju in comment #22. However, I've stomped into the following issue: precondition: 1- Linking with Editor is enabled at workbench startup 2- Java Editor is active on a Test Case actions: 3- user activates another Compilation Unit Editor 4- the Content Outline view shows content for the compilation unit selected above 5- user clicks on another method of the class 6- the active editor gets back to the Test Case (as in step 2) From what I could see when putting a breakpoint in TestViewer#selectionChanged(IWorkbenchPart, ISelection) is that it seems as if the Content Outline sent a selection change notification with a selection matching the previous test case, not the current compilation unit.. Can you reproduce it ? Can you see something wrong in my patch about this, or could you give me some advice ? Thanks
Thanks Xavier for the patch. With this patch, the feature works as expected. I could not reproduce the issue you encountered in comment 24. The code needs refactoring and clean up. 1. Update the copyright year of all the modified files to the current year. e.g. "Copyright (c) 2000, 2010" should be "Copyright (c) 2000, 2014". 2. You have added 2 new APIs in ITestCaseElement. Is it required? Saw the usage in TestViewer#findTestCaseElement, shouldn't the testcase name and the method name check sufficient in this case? 3. TestCaseElement implements IAdaptable. I might have overlooked some case and hence did not understand why was it needed or which feature will fail if it is removed? 4. The implementation of getJavaType() in TestCaseElement looks complicated. In #getAdapter() you have written a simple code to fetch the IType, why was that not used in #getJavaType()? final IJavaProject launchedProject= getTestRunSession().getLaunchedProject(); fJavaType= launchedProject.findType(this.getTestClassName()); 5. Same with #getJavaMethod(). Can't we replace it with: if (fJavaMethod == null) { String testMethodName= getTestMethodName(); if (getJavaType() != null) { IMethod method= fJavaType.getMethod(testMethodName, new String[0]); if (method != null && method.exists()) { fJavaMethod= method; } } } return fJavaMethod; To handle the test case with parameters, #getTestMethodName should take care of '[' just like it has currently handled '@' and '('. In that case #findJavaMethod can be removed. 6. TestViewer, dead code in LN:186 7.TestViewer#handleTestSelected needs code clean up. EditorUtility.revealInEditor is invoked twice in this method, that should be taken care off and also remove the unwanted/commented code. 8. In some places the coding convention is not followed. For e.g. if (condition), note the space between if and '('. Format (Ctrl+Shift+F) the modified code before creating a patch. As we do not have JUnits for this feature, I might have overlooked some cases. Let me know if you agree with the above points and if so an updated patch would help.
@Dani In Javadoc View, we had the feature where the Link With Editor shows a cross if the view is not in sync with editor. Should the same be implemented in this case also? Xavier with his patch is saving the state of the action(enabled/disabled) in preference store. This is not done in the case of Javadoc View. Is this fine?
@Manju, Thanks a lot for your last review, I'm glad the code now works as expected ;-) I'll take care of all the review items you provided me with in comment #25. Best regards,
(In reply to Manju Mathew from comment #26) > @Dani > In Javadoc View, we had the feature where the Link With Editor shows a cross > if the view is not in sync with editor. Should the same be implemented in > this case also? I didn't follow all the discussion in the bug, but "yes", if the linking is more like the one in our info views and not like 'Package Explorer' <-> Editor, and if it can get out of sync, then it should be handled like in the info views. > 2. You have added 2 new APIs in ITestCaseElement. Is it required? Saw the > usage > in TestViewer#findTestCaseElement, shouldn't the testcase name and the > method name check sufficient in this case? Those are more like helper methods and don't belong into the API. > 3. TestCaseElement implements IAdaptable. I might have overlooked some case > and Also sounds unnecessary for implementing just the linking. But again, I didn't follow all the comments.
@Xavier When we switch editor and JUnit View is active, then the selection in JUnitView will not be in sync with the active editor. In such case the "Link With Editor" image should show a different image indicating out of sync. Check Javadoc View to see the behavior. This bug will not be complete without this feature. If interested you can work on it once the current feature patch is complete.
Created attachment 240412 [details] New patch with code reviewed and 'out-of-sync' feature implemented @manju, @dani, I'm submitting this new patch in which I took care of (hopefully) all comments you made, and in which I also implemented the missing 'out-of-sync' button state feature. To be more accurate, I'm provinding you with inline comments: > 1. Update the copyright year of all the modified files to the current year. > e.g. "Copyright (c) 2000, 2010" should be "Copyright (c) 2000, 2014". Done. >> 2. You have added 2 new APIs in ITestCaseElement. Is it required? Saw the >> usage > in TestViewer#findTestCaseElement, shouldn't the testcase name and >>the method name check sufficient in this case? > >Those are more like helper methods and don't belong into the API. Ok, I removed those 2 methods from ITestCaseElement but kept them in the internal implementation (see item below for more info). The side effect is that I now need to cast ITestCaseElement to TestCaseElement in the TestViewer to access those helper methods. > 3. TestCaseElement implements IAdaptable. I might have overlooked some case > and hence did not understand why was it needed or which feature will fail if > it is removed? Implementing IAdaptable was required because when the selected element sent to the Selection Manager is a TestCaseElement, the other info view try to convert it into an IJavaElement using the IAdaptable#getAdapter(Class) method. If you remove the 'implements IAdaptable' signature on the type definition, you'll see that the Javadoc and Declaration view don't get updated when the selection changes in the JUnit View. This was a requirement express by Manju in comment #13 (item 4). > 4. The implementation of getJavaType() in TestCaseElement looks complicated. > In #getAdapter() you have written a simple code to fetch the IType, why was > that not used in #getJavaType()? > ... Right, the code was simplified to: getTestRunSession().getLaunchedProject().findType(getTestClassName()); > 5. Same with #getJavaMethod(). Can't we replace it with: > ... > To handle the test case with parameters, #getTestMethodName should take care > of '[' just like it has currently handled '@' and '('. > In that case #findJavaMethod can be removed. Right, thanks for pointing me to #getTestMethodName, it actually simplifies much the code (and makes total sense) ! > 6. TestViewer, dead code in LN:186 Done. > 7.TestViewer#handleTestSelected needs code clean up. > EditorUtility.revealInEditor is invoked twice in this method, that should be > taken care off and also remove the unwanted/commented code. Done. > 8. In some places the coding convention is not followed. For e.g. if > (condition), note the space between if and '('. Format (Ctrl+Shift+F) the > modified code before creating a patch. Done. >>> @Dani >>> In Javadoc View, we had the feature where the Link With Editor shows a cross >>> if the view is not in sync with editor. Should the same be implemented in >>> this case also? >> >>I didn't follow all the discussion in the bug, but "yes", if the linking is >>more like the one in our info views and not like 'Package Explorer' <-> >>Editor, and if it can get out of sync, then it should be handled like in the >>info views. >When we switch editor and JUnit View is active, then the selection in >JUnitView will not be in sync with the active editor. In such case the "Link >With Editor" image should show a different image indicating out of sync. Check >Javadoc View to see the behavior. >This bug will not be complete without this feature. If interested you can work >on it once the current feature patch is complete. This patch also includes this feature. Please, let me know if this is OK for you or if more love needs to be given to the patch ;-)
Thanks Xavier. Couple of changes are needed: 1. Looks like before creating the patch you formatted the complete file, hence i was not able to identify which lines were actually modified and which lines were formatted. Set the preference in such a way that only the edited lines are formatted. Open Eclipse > Windows> Preferences > Save Actions > Format source code , here choose "Format edited lines". This way every time you make changes and save, only the edited lines are formatted. Another preferred way is to select the lines that needs to be formatted and then press Ctrl+Shift+F. Also before creating a patch, compare the local file with the head revision to make sure you do not have unwanted changes in the patch. 2. Enable LWE(Link With Editor) when the JUnit view is activated. This is the default behavior in Javadoc View also. 3. No need to store the state of the LWE action in the preference store, again follow what was done in Javadoc view. 4. JUnit view is in sync, now switch to Package Explorer View and switch back to JUnit view, LWE shows out of sync which is wrong. 5. JUnit view is in sync, now switch to another file in the editor area, LWE goes out of sync, which is right. Select a testcase method in the Junit view, the test file is activated in the editor area, but LWE still shows out of sync. 6. Open a test file from a jar, i.e you will see the .class file. Now execute the testcases from the binary test file. After the test are completed select a test method from the JUnit View or Outline view, NPE is thrown: java.lang.NullPointerException at org.eclipse.jdt.internal.junit.ui.TestViewer.handleTestSelected(TestViewer.java:853) at org.eclipse.jdt.internal.junit.ui.TestViewer.handleSelected(TestViewer.java:344) at org.eclipse.jdt.internal.junit.ui.TestViewer.access$0(TestViewer.java:334) at org.eclipse.jdt.internal.junit.ui.TestViewer$TestSelectionListener.selectionChanged(TestViewer.java:93) at org.eclipse.jdt.internal.ui.viewsupport.SelectionProviderMediator.fireSelectionChanged(SelectionProviderMediator.java:131) at org.eclipse.jdt.internal.ui.viewsupport.SelectionProviderMediator.doSelectionChanged(SelectionProviderMediator.java:112) at org.eclipse.jdt.internal.ui.viewsupport.SelectionProviderMediator$InternalListener.selectionChanged(SelectionProviderMediator.java:40) at org.eclipse.jface.viewers.Viewer$2.run(Viewer.java:167) at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:42) at org.eclipse.ui.internal.JFaceUtil$1.run(JFaceUtil.java:49) at org.eclipse.jface.util.SafeRunnable.run(SafeRunnable.java:178) As you said implementing IAdaptable is required to keep the syncing with other info views.
Manju Sorry about the patch formatting. The last one I submitted contained a lot of changes in my code, that also might explain why you could hardly see the changes. Anyway, I applied the settings you recommended. As I'm looking at the Javadoc View (it's actually a good example of how LWE works), I noticed the following behaviour: if I select a class in the package explorer, the Javadoc View keeps in sync and shows the javadoc for the selected class (or has a "out-of-sync" state if the selected resource is not a Java Element), even if the current editor displays another resource. So, my question is: should the JUnit View behave the same way as the Javadoc and move its selection to the corresponding TestElement when the selection changes in the Package Explorer (or go "out-of-sync" if there's no such TestElement), or should it always change its state to "out-of-sync" ? Thanks for your feedback.
(In reply to Xavier Coulon from comment #32) > As I'm looking at the Javadoc View (it's actually a good example of how LWE > works), Note that in Javadoc View the action is "Link with Selection" and not "Link with Editor". I referred Javadoc View so that you can have a better understanding of the action icon changing its state. > I noticed the following behaviour: if I select a class in the > package explorer, the Javadoc View keeps in sync and shows the javadoc for > the selected class (or has a "out-of-sync" state if the selected resource is > not a Java Element), even if the current editor displays another resource. > So, my question is: should the JUnit View behave the same way as the Javadoc > and move its selection to the corresponding TestElement when the selection > changes in the Package Explorer (or go "out-of-sync" if there's no such > TestElement), or should it always change its state to "out-of-sync" ? > Javadoc View is doing its job of "Link with Selection", the selection can be from the Package Explorer View or the current active editor. In the case of JUnit View, the action is "Link With Editor", so at any point when the JUnit View is active(visible to user) and the JUnit View is populated with a test result, then it should show the valid state wrt the currently active editor.
@Manju, Thanks for the clarification. I'll update the patch with your previous comments.
Created attachment 241975 [details] New Patch after review, with support for Parameterized Tests @Manju, I applied all the changes after your last code review. Since the patch for https://bugs.eclipse.org/bugs/show_bug.cgi?id=102512 has been applied, I also added support for Parameterized Tests: - TestSuiteElement implements IAdaptable - TestCaseElement#getJavaType() moved to TestElement because it's also used in TestSuiteElement When the Java Type cannot be deduced from the TestSuiteElement (in particular, in the case of a Parameterized Test), the code looks for the single child TestCaseElement and returns its Java Type. This means that in the TestViewer, user can click on a TestSuiteElement (eg: "[0]" if no custom name was specified in the @Parameters annotation of the method that generates the test data) and the associated Java Method is revealed in its Java Editor. Also, when the user selected a TestSuiteElement that matches a Java Type (in the case of a basic test class for example), then the corresponding Java Type is revealed in its Java Editor. Please, let me know what you think about this new patch. Thanks.
Created attachment 242004 [details] New Patch after review, with support for Parameterized Tests and without formatting issues Submitting again the patch after I removed all unnecessary changes due to code formatting performed by mistake. Also fixed a couple of remaining issues caught during the process. This patch replaces the previous ones.
@Manju, Can we carry on with the review to see if this patch can be included in one of the next milestones of Eclipse Mars? thanks
(In reply to Xavier Coulon from comment #36) > Created attachment 242004 [details] [diff] - This patch does not fix the behavior expected in comment #22. - Add some test classes to a test suite, run the suite. a) Double click on a test class name in the JUnit view to open the file => the LWE icon is crossed, which is a bug. b) In the Java editor, select the test class name, it is not linked with the test class in JUnit view and LWE icon is crossed. - Still all the files don't have updated copyright year and header (eg: TestSuiteElement.java). - Update @since tags in the patch. Follow: https://wiki.eclipse.org/JDT_UI/How_to_Contribute
Created attachment 248907 [details] Update patch with missing @since and file headers Added missing file headers and @since tags on public methods.
Created attachment 248908 [details] Update patch with updated @since tag and file headers Added @since tags on public methods and modified to @since 3.8 on public methods in org.eclipse.jdt.junit to match the bundle version.
@Noopur, I'm really confused, because I cannot reproduce the problems that you reported. Could you provide me with the test project that you used, along with detailed steps to reproduce the bugs ? In the mean time, I submitted another patch that should hopefully all the file headers and @since tags that were missing/outdated. Thanks
Created attachment 248945 [details] In reply to Xavier Coulon from comment #41 (In reply to Xavier Coulon from comment #41) > @Noopur, > > I'm really confused, because I cannot reproduce the problems that you > reported. > Could you provide me with the test project that you used, along with > detailed steps to reproduce the bugs ? > In the mean time, I submitted another patch that should hopefully all the > file headers and @since tags that were missing/outdated. > > Thanks Attached the test project and screenshots with the issues. Steps are already mentioned in detail in comment #38. Note how Outline and Javadoc views are updated and the JUnit view is not updated in the specified cases when selection is changed.
(In reply to Noopur Gupta from comment #42) > Created attachment 248945 [details] > In reply to Xavier Coulon from comment #41 > > (In reply to Xavier Coulon from comment #41) > > @Noopur, > > > > I'm really confused, because I cannot reproduce the problems that you > > reported. > > Could you provide me with the test project that you used, along with > > detailed steps to reproduce the bugs ? > > In the mean time, I submitted another patch that should hopefully all the > > file headers and @since tags that were missing/outdated. > > > > Thanks > > Attached the test project and screenshots with the issues. Steps are already > mentioned in detail in comment #38. > Note how Outline and Javadoc views are updated and the JUnit view is not > updated in the specified cases when selection is changed. Thanks, I'll try with the sample project you just provided me with.
Created attachment 249171 [details] Patch proposal with bugfixes @Noopur, Thanks for the sample project, it was very helpful to fix the bugs you reported in your previous comments. Please find now another version of the patch, with hopefully all bugs cleared. I also edited the @since and headers in the proposed files, but I noticed that the org.eclipse.jdt.junit.core bundle is still in version 3.7 whereas org.eclipse.jdt.junit is in version 3.8, so my @since tags are set accordingly. If you think that org.eclipse.jdt.junit.core version should be set to 3.8, I'll update the @since tags to match the version. Please, let me know if you find more bugs. Thanks for your patience and your feedbacks.
(In reply to Xavier Coulon from comment #44) > Created attachment 249171 [details] > Patch proposal with bugfixes Found the following issue with this patch: 1. In the project from comment #42, run AllTests. 2. Open "TP1" and add a field "String s;" before the test methods. 3. Place the caret in the editor at "testGetStr". 4. Now click on "String s;" to move the caret there. => Bug is that the caret moves to the class name "TP1" on its own from the field. Another issue: When both "TP1" and "TP2" editors are open and the selection is changed from "TP1" to "TP2" in Package Explorer view, the active (visible) editor changes to "TP2". But the selection in JUnit view is not updated to reflect the current editor (as requested in comment #33). See how it is updated in Project Explorer. In general, when JUnit view is visible and has LWE enabled, by any means when the active editor or selection in the editor changes, JUnit view should be updated to reflect the state - highlight the current element if it is present in the JUnit view result or show out-of-sync symbol.
@Noopur Gupta, Thanks for spotting those 2 bugs. I could reproduce them on my machine and I'm going to fix them.
Created attachment 251221 [details] Patch proposal with bugfixes This patch fixes the 2 problems reported in comment#45 from Noopur Gupta Please note the following behaviour change: when opening a compilation unit in the project explorer, the focus in the editor is on the package declaration, not on the primary type. This means that the JUnit LWE button will appear in 'broken sync' state until the user selects the type or a test method in the editor (or selects a testcase suite or element in the JUnit view, or a corresponding type or method in outline view or in the project explorer). This behaviour is what seems to be already the case in the Content Outline view: the type is not selected when the editor opens because the selection is on the package declaration. Please, let me know if this behavior is acceptable or not. Thanks.
@Noopur Gupta, Would you have time to review the code so that we can try to include it in Mars M7 ?
(In reply to Xavier Coulon from comment #47) > Created attachment 251221 [details] [diff] > Patch proposal with bugfixes I haven't looked at the code yet. When I run any JUnit after applying this patch, when JUnit view is open with the LWE button not pressed, I get these exceptions in Error log on selecting different elements: - Working set in Package explorer: java.lang.ClassCastException: org.eclipse.ui.internal.WorkingSet cannot be cast to org.eclipse.jdt.core.IJavaElement at org.eclipse.jdt.internal.junit.ui.TestViewer.selectionChanged(TestViewer.java:935) ... - Log entry in Error log: java.lang.ClassCastException: org.eclipse.ui.internal.views.log.LogEntry cannot be cast to org.eclipse.jdt.core.IJavaElement at org.eclipse.jdt.internal.junit.ui.TestViewer.selectionChanged(TestViewer.java:935) ... - Test suite in JUnit view: java.lang.ClassCastException: org.eclipse.jdt.internal.junit.model.TestSuiteElement cannot be cast to org.eclipse.jdt.core.IJavaElement at org.eclipse.jdt.internal.junit.ui.TestViewer.selectionChanged(TestViewer.java:935) ... etc. For the next patch, please revisit the entire code and do some testing to verify that all scenarios discussed in this bug are working.
@Noopur Gupta Thanks for the feedback. I have a suite of tests that cover all the bugs that you reported before, but sadly I missed some cases. I'll make sure the ones you reported today are added and pass.
@Noopur Gupta Aside from the new bugs you reported earlier today, can you confirm me that the bugs you reported during your previous reviews have been fixed ? Thanks in advance
Created attachment 251985 [details] Bug - Screenshot (In reply to Xavier Coulon from comment #51) > @Noopur Gupta > > Aside from the new bugs you reported earlier today, can you confirm me that > the bugs you reported during your previous reviews have been fixed ? > > Thanks in advance I just started verifying the scenarios based on your latest patch and after ignoring the CCEs reported in comment #49, found some more basic bugs: First bug: - Take project from comment #42 and fix it so that all tests pass in it. - Open and run "AllTests". - Expand all the nodes in JUnit view. - Click on "testSetStr" in TP1. => The test method is selected in editor but the selection in JUnit view is moved to the test class. See attached screenshot. Second bug: - Select a test class in Package Explorer with its LWE enabled. The class name is selected in the open editor and JUnit view. But the JUnit view shows out of sync. Third bug: - Comment #45: If the focus was on field "String s;", then change in selection in Package explorer does not update the selection in JUnit view. As mentioned multiple times (see comment #22 also), before submitting the next patch, please revisit the entire code and do proper testing and also make sure that all scenarios discussed in this bug are working. I will verify all the scenarios and do a code review only after I receive the next patch from you which works as per the required functionality. You mentioned that you have a suite of tests that cover all the bugs that were reported here. Please attach it here as a separate patch so that manual testing can be avoided. Also, do not include .gitignore files in the patch. And mark any existing patch as obsolete when you attach the new patch.
@Noopur Gupta Thanks for the extended review. I'm going to add more tests in my test suite to cover the bugs that you reported. You mentionned in comment #52 the following bug (amongst others): "Second bug: - Select a test class in Package Explorer with its LWE enabled. The class name is selected in the open editor and JUnit view. But the JUnit view shows out of sync." This is a behaviour that I reported in comment #47 and for which I was asking for feedback from you. Can you provide me with guidelines for this particular case ? Thanks.
Created attachment 252025 [details] Screenshot - comment 52, second bug (In reply to Xavier Coulon from comment #53) > You mentionned in comment #52 the following bug (amongst others): > "Second bug: > - Select a test class in Package Explorer with its LWE enabled. The class > name is selected in the open editor and JUnit view. But the JUnit view shows > out of sync." > > This is a behaviour that I reported in comment #47 and for which I was > asking for feedback from you. Can you provide me with guidelines for this > particular case ? The second bug in comment #52 is not about when the focus in the editor is on package declaration (as in comment #47). Even when the focus is on class name in the editor, JUnit view shows out-of-sync (see attached screenshot). For the case in comment #47, to decide on the sync behavior of JUnit view, you can check how the same is done in Javadoc view. I feel that adding out-of-sync behavior is complicating this patch. Hence, I would suggest that you provide a patch with just LWE first and you can work on the sync behavior after LWE is committed. The same was also suggested in comment #29.
Xavier, can you update your Gerrit review with your latest changes? Gerrit seems still to show an older version: https://git.eclipse.org/r/#/c/21752/ Looks like you provided initially a Gerrit review but afterwards continued to use patches.
Gautier, maybe you can help finishing this? I see you are very actively working on better JUnit support in the IDE.
