Comment 1 Brock Janiczak 2008-02-19 22:34:42 EST

I have some code to sort the results by execution time, name or execution order.  It actually looks very confusing as the currently running test pointer jumps around a lot.  Would it make sense to delay sorting until the test execution is completed?

Comment 2 Dani Megert 2008-02-20 02:18:32 EST

Perfect for me. I only want to see at the end which tests need lots of time.

Comment 3 Martin Aeschlimann 2008-02-20 03:24:02 EST

I guess this feature only makes sense in the flat view.

Note that we have no sorting possibilities at the moment, neither by name, not by success/failure.

So would be a bit careful and would not add a single sort feature working for only one of the views. But of course I can be convinced...!

Comment 4 Dani Megert 2008-02-20 03:28:00 EST

>I guess this feature only makes sense in the flat view.
Good enough. I initially suggested a filter of which we already have some but Markus said the sorting would be simpler to do.

Comment 5 David Loose 2008-04-18 11:01:13 EDT

I would also like to see the ability to sort by the name of the unit test class in the JUnit view. 

Comment 6 Aaron Digulla 2010-07-30 07:59:03 EDT

(In reply to comment #5)
> I would also like to see the ability to sort by the name of the unit test class
> in the JUnit view.

With JUnit 4, it becomes increasingly complex to locate a test (for example to run it again with the same config).

The built-in search of the view doesn't really help since I have to type in the whole name of the test class. At least "*name" didn't work.

Comment 7 Markus Keller 2011-02-24 09:50:36 EST

- sorting should work for both tree and flat mode
- sorting should only be applied after test run has ended (show execution order before)
- sort actions should go to the view menu, not the tool bar

Comment 8 Dmitry Katsubo 2012-12-13 07:15:06 EST

I vote for sorting by class name and/or package. Otherwise locating a particular test (e.g. to rerun) requires scrolling the whole list.

Comment 9 sandra lions 2014-06-04 04:48:17 EDT

You can assign the issue to me.

Comment 10 sandra lions 2014-06-16 04:13:37 EDT

Created attachment 244275 [details]
Unit Test + Patch

This contribution complies with http://www.eclipse.org/legal/CoO.php

Comment 11 Martin Mathew 2014-06-24 02:49:08 EDT

(In reply to sandra lions from comment #10)
> Created attachment 244275 [details] [diff]
> Unit Test + Patch
The patch looks good. Some comments:
1. Copyright year of all files were not updated. For new files, list yourself "and others" instead of "IBM and others" in the first line.
2. When sorting is based on 'Name' the result should be a=A < B=b, we need to ignore case. Hence as already used in other places like NewKeysPreferencePage#compareColumn, in TestNameComparator#compare we can use getComparator().compare(testName1, testName2)

@Markus: Is there any disadvantage for this approach?

Comment 12 Martin Mathew 2014-06-24 03:13:36 EDT

One more point to take care, make TestNameComparator and TestExecutionTimeComparator as private final class and not static class.

Comment 13 sandra lions 2014-06-24 09:34:53 EDT

Created attachment 244471 [details]
Unit Test + Patch version 2

Thanks for your review Manju.
Here attached new patch taking into account your feedback.
Note that I've also updated the unit test in order to check that names sorting is not case sensitive.

Comment 14 Martin Mathew 2014-06-25 01:55:00 EDT

Thanks Sandra. The patch is released to 'marsTemp' as: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?h=marsTemp&id=bec0bf9b436525ac586e8f806ce9b72a61254ccd

Comment 15 Robin Stocker 2014-06-25 09:16:06 EDT

Nice work! May I interest you in other improvements to Eclipse's JUnit integration ;)? See bug 208862 and bug 111126.

Comment 16 Markus Keller 2014-06-25 13:34:46 EDT

This is not quite done yet.

* Menu labels need mnemonics (e.g "&Sort By")

* The natural order should be the first in the menu

* Shouldn't the sort order by execution time be descending (longest test first)? See comment 2. 

* The JUnit view supports multiple currently running test sessions. We should not disable the Sort By actions when the user is looking at a test run that has already ended.

The working of the toggle actions needs more thought in general. The idea in comment 7 was that the sorting is always by execution order while the test is running. This is not the case after the fix, since the order can already be different before the run.
- There's no point in just disabling the actions.
- With Sort By > Name, the view behaves very erratically when Scroll Lock is disabled: Test suites are expanded/collapsed, the current test jumps around, and the view is of no use.
- With Sort By > Execution Time, the sorting is wrong while the tests are running. We won't go for an "easy" solution that just re-sorts the whole tree/list whenever something changes, since that doesn't scale for big test suites.

* Coding problems:
- SortingCriteria is plural, but should be a singular (SortingCriterion)
- Stylistic nit: Our general style is to use "i++" in "for" loops. You get that when you use the "for" template (in Content Assist) to generate a loop.

Comment 17 Markus Keller 2014-06-25 13:46:08 EDT

Created attachment 244528 [details]
Example Test to play around

Comment 18 Markus Keller 2014-06-25 14:06:26 EDT

(In reply to Manju Mathew from comment #14)
> Thanks Sandra. The patch is released to 'marsTemp' as:
> http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/
> ?h=marsTemp&id=bec0bf9b436525ac586e8f806ce9b72a61254ccd

I didn't push this commit back to the master branch, so that we can get a cleaner Git history where comment 16 is included in the commit.

Comment 19 Markus Keller 2014-06-30 06:52:42 EDT

The "Next/Previous Failed Test" buttons (Ctrl+./,) should also respect the current order. With the commit from comment 14, they always use execution order.

Comment 20 sandra lions 2014-07-03 10:16:23 EDT

(In reply to Markus Keller from comment #16)

> * The JUnit view supports multiple currently running test sessions. We
> should not disable the Sort By actions when the user is looking at a test
> run that has already ended.
> 
> The working of the toggle actions needs more thought in general. The idea in
> comment 7 was that the sorting is always by execution order while the test
> is running. This is not the case after the fix, since the order can already
> be different before the run.
> - There's no point in just disabling the actions.
> - With Sort By > Name, the view behaves very erratically when Scroll Lock is
> disabled: Test suites are expanded/collapsed, the current test jumps around,
> and the view is of no use.
> - With Sort By > Execution Time, the sorting is wrong while the tests are
> running. We won't go for an "easy" solution that just re-sorts the whole
> tree/list whenever something changes, since that doesn't scale for big test
> suites.

Hi Markus,
I’ve started to implement your comments and I have a question.
I understand that when starting a test run, the sorting criteria must be set to EXECUTION ORDER. This was missing in my patch.
Then, to keep this value unchanged during the test run, we disable the Sort By actions and enable them back when the test run is over.
Is it correct understanding ?

Thanks,
Sandra

Comment 21 Martin Mathew 2014-07-03 20:46:45 EDT

(In reply to sandra lions from comment #20)
> Hi Markus,
> I’ve started to implement your comments and I have a question.
> I understand that when starting a test run, the sorting criteria must be set
> to EXECUTION ORDER. This was missing in my patch.
> Then, to keep this value unchanged during the test run, we disable the Sort
> By actions and enable them back when the test run is over.
> Is it correct understanding ?
> 
> Thanks,
> Sandra

The problem with disabling Sort By action when a test is being executed is that, user will not be able to sort the results of an already completed test from history.

Comment 22 sandra lions 2014-07-11 03:09:32 EDT

(In reply to Manju Mathew from comment #21)
> (In reply to sandra lions from comment #20)
> > Hi Markus,
> > I’ve started to implement your comments and I have a question.
> > I understand that when starting a test run, the sorting criteria must be set
> > to EXECUTION ORDER. This was missing in my patch.
> > Then, to keep this value unchanged during the test run, we disable the Sort
> > By actions and enable them back when the test run is over.
> > Is it correct understanding ?
> > 
> > Thanks,
> > Sandra
> 
> The problem with disabling Sort By action when a test is being executed is
> that, user will not be able to sort the results of an already completed test
> from history.

Hi Manju,
To cover the use case you describe (sorting an already completed test run session while another one is running) , we need to link the "Sort By" menu items to a single test run session.
This is not similar to what other menu items do (Show Test in Hierarchy, Show Execution Time...) which apply to all test run sessions.
Do you think the use case justifies to break this similarity ?   

Thanks,
sandra

Comment 23 Markus Keller 2014-07-14 10:08:10 EDT

I think fixing the updating issues mentioned in comment 1 and comment 16 would be a lot of work and would need a lot of UI fine-tuning. It's much easier to always use execution order while tests are running, and only apply the sorting after the test run has ended.

Let's first try the simplest solution and see how it feels:
(1) While tests are running, keep the execution order (even if a different order was selected before or during the test run).
(2) Once the tests are done and the view title is not shown in italics ("busy") any more, apply the current sort order.
(3) When the user switches the view to another test run (via the history button), apply rules (1) and (2), depending on the current state of the shown test run.

Comment 24 sandra lions 2014-07-15 09:19:41 EDT

Created attachment 245073 [details]
Unit Test + Patch version 3

Take into account Markus comments.
I've played with the test example attached to this bug and the simple solution looks satisfactory to me.

Comment 25 Martin Mathew 2014-07-23 22:37:29 EDT

(In reply to sandra lions from comment #24)
> Created attachment 245073 [details]
> Unit Test + Patch version 3
I am unable to apply this as a patch. Can you check and upload a .patch file?

Comment 26 Martin Mathew 2014-07-28 00:24:24 EDT

(In reply to Manju Mathew from comment #25)
> I am unable to apply this as a patch. Can you check and upload a .patch file?
With the help from Markus i was able to apply the fix.

Comment 27 Martin Mathew 2014-07-29 03:20:33 EDT

(In reply to sandra lions from comment #24)
> Created attachment 245073 [details] [diff]
> Unit Test + Patch version 3

1. I encountered the below NPE while testing with this patch, unfortunately i am unable to reproduce the exception:
org.eclipse.swt.SWTException: Failed to execute runnable 

(java.lang.NullPointerException)
	at org.eclipse.swt.SWT.error(SWT.java:4441)
	at org.eclipse.swt.SWT.error(SWT.java:4356)
	at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages

(Synchronizer.java:139)
	at org.eclipse.swt.widgets.Display.runAsyncMessages(Display.java:4147)
	at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3764)
	at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine$9.run

(PartRenderingEngine.java:1151)
	at org.eclipse.core.databinding.observable.Realm.runWithDefault

(Realm.java:332)
	at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.run

(PartRenderingEngine.java:1032)
	at org.eclipse.e4.ui.internal.workbench.E4Workbench.createAndRunUI

(E4Workbench.java:148)
	at org.eclipse.ui.internal.Workbench$5.run(Workbench.java:636)
	at org.eclipse.core.databinding.observable.Realm.runWithDefault

(Realm.java:332)
	at org.eclipse.ui.internal.Workbench.createAndRunWorkbench

(Workbench.java:579)
	at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:150)
	at org.eclipse.ui.internal.ide.application.IDEApplication.start

(IDEApplication.java:135)
	at org.eclipse.equinox.internal.app.EclipseAppHandle.run

(EclipseAppHandle.java:196)
	at 

org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication

(EclipseAppLauncher.java:134)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start

(EclipseAppLauncher.java:104)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run

(EclipseStarter.java:382)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run

(EclipseStarter.java:236)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke

(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke

(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:483)
	at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:648)
	at org.eclipse.equinox.launcher.Main.basicRun(Main.java:603)
	at org.eclipse.equinox.launcher.Main.run(Main.java:1465)
	at org.eclipse.equinox.launcher.Main.main(Main.java:1438)
Caused by: java.lang.NullPointerException
	at org.eclipse.jdt.internal.junit.ui.TestRunnerViewPart.updateSortByMenu

(TestRunnerViewPart.java:1033)
	at 

org.eclipse.jdt.internal.junit.ui.TestRunnerViewPart.setActiveTestRunSession

(TestRunnerViewPart.java:1605)
	at org.eclipse.jdt.internal.junit.ui.TestRunnerViewPart.access$2

(TestRunnerViewPart.java:1530)
	at org.eclipse.jdt.internal.junit.ui.TestRunnerViewPart

$TestRunSessionListener$1.run(TestRunnerViewPart.java:688)
	at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:35)
	at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages

(Synchronizer.java:136)
	... 24 more

2. Soon after Eclipse runtime is up, if user switch to JUnit view, all the options under 'Sort By' are enabled and selection has no effect. The Sort By actions needs to be enabled only if we have at least one test result available. 

3. 'Sort By' is assigned a mnemonic, similarly the sub menu items under 'Sort By' also should have mnemonics assigned.

4. Comment 16 from Markus on coding practice is not taken care in TestRunnerViewPart:
for (int i= 0; i < fToggleSortingActions.length; ++i)

5. In TestRunnerViewPart#updateSortByMenu the logic can be simplified. The statements within the inner 'if' and outer 'else' are the same and hence can be combined.

Comment 28 Markus Keller 2014-07-29 06:55:35 EDT

(In reply to Manju Mathew from comment #27)
> 2. Soon after Eclipse runtime is up, if user switch to JUnit view, all the
> options under 'Sort By' are enabled and selection has no effect. The Sort By
> actions needs to be enabled only if we have at least one test result
> available. 

Maybe the user just wants to pre-set the sorting order for the next test run? Or she wants to switch the eventual order while tests are still running? I don't think we should ever disable the actions. While tests are running, the view title is italic ("busy"), so it's OK to let the user use the actions and only update the order after the test run has ended.

Comment 29 sandra lions 2014-09-11 04:48:57 EDT

Created attachment 246948 [details]
Unit Test + Patch version 4

Following Markus feedback, the "Sort By" command is always enabled.

Comment 30 Markus Keller 2014-09-15 19:22:36 EDT

Sorry, I have to move this to M3. Found a few more problems during the review:

* TestRunnerViewPart:
- TAG_SORTING_CRITERION= "SortingCriterion";
=> tag should start lowercase like all the other tags

- better store ordinal of Enum, not name (same as everywhere else; uses less space; not vulnerable to renaming of the constant; no implementation lock-in)

- setSortingCriterion(SortingCriterion):
  - instead of 'isX() == false', use '!isX()'
  - condition needs !fTestRunSession.isStarting() as well

- the asyncExecs don't look right. I think the setSortingCriterion(..) calls can be included in existing runnables that are posted to the UI thread, so we can avoid the uncertain execution time and the isDisposed() checks that would be necessary for asyncExecs (e.g. when the user tries to close the view right when the test run session ends).

* TestViewer#setSortingCriterion(SortingCriterion)
- typo: viewComparator => viewerComparator

- we should only set the comparator on the viewer that is currently shown. The whole implementation of TestViewer keeps the two viewers separate and only ever updates the viewer that is actually shown. Setting a comparator on a bigger table/tree can be quite expensive. It's probably also faster to call
		fTableViewer.getControl().setRedraw(false/true);
before/after setting the comparator.

Comment 31 Noopur Gupta 2014-11-06 05:23:13 EST

*** Bug 449783 has been marked as a duplicate of this bug. ***

Comment 32 Patrik Nandorf 2015-01-22 03:41:01 EST

Any update?

Comment 33 Markus Keller 2015-01-23 11:16:04 EST

Sorry, the latest patch still needs some work. I don't have time to finish it up for M5, and Sandra is no longer working on this.

Comment 34 Markus Keller 2015-03-17 11:37:40 EDT

Tentatively targeting M7, but may have to move it again. If someone wants to help speed it up, see comment 30. Unfortunately, it doesn't look like this can be finished with a few trivial changes.

Comment 35 Eclipse Genie 2020-02-26 09:27:03 EST

New Gerrit change created: https://git.eclipse.org/r/158405

Comment 36 Gautier de SAINT MARTIN LACAZE 2020-02-26 09:35:36 EST

Hello Markus, 

I started to work on this especially your review in https://bugs.eclipse.org/bugs/show_bug.cgi?id=219466#c30

I didn't start to work on asyncExec problems. I'm not sure I understand the problem here.

Feel free to comment here or directly on gerrit.

Comment 37 Stephan Herrmann 2020-02-26 14:34:19 EST

(In reply to Gautier de SAINT MARTIN LACAZE from comment #36)
> Hello Markus, 

FWIW, I don't think Markus is still reading here.

Comment 38 Gautier de SAINT MARTIN LACAZE 2020-05-05 11:40:17 EDT

@Stephan Indeed after some digging I realize why you said Markus will not read this thread. 

Do you have an idea about what is the problem with `asyncExec`? I'm not sure I have enough knowledge on this to fully understand the problem.

Comment 39 Gautier de SAINT MARTIN LACAZE 2020-06-11 09:26:54 EDT

Hello team, 

Is anybody available to review this little change?

Thanks for your work.

Comment 40 Gautier de SAINT MARTIN LACAZE 2020-08-13 07:53:42 EDT

Hello Noopur any news about gerrit patch?

Comment 41 Noopur Gupta 2020-08-13 09:10:54 EDT

Jeff/Roland, can one of you please review the patch?

Comment 42 Jeff Johnston 2020-08-14 20:02:07 EDT

(In reply to Noopur Gupta from comment #41)
> Jeff/Roland, can one of you please review the patch?

Hi Noopur, I finally was able to take a proper look at it.  I was having issues testing due to some module issues (e.g. com.sun.jdi) but it appears to have been solved in the I-builds.  I was able to verify the new sort and that it did not occur during execution.  I uploaded a modification for the new TestSorting test and will wait for the gerrit job to verify so I can verify the test ran.

Comment 43 Eclipse Genie 2020-08-15 00:50:18 EDT

Gerrit change https://git.eclipse.org/r/c/jdt/eclipse.jdt.ui/+/158405 was merged to [master].
Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=e2195d71e46af4a29950762d324ff285ae4972dd

Comment 44 Jeff Johnston 2020-08-15 00:51:10 EDT

Released for 4.17M3

Comment 45 Noopur Gupta 2020-08-17 03:38:47 EDT

Thanks!

Please add a N&N entry for M3.

Comment 46 Gautier de SAINT MARTIN LACAZE 2020-08-18 05:05:11 EDT

I hope I can do that today.

Comment 47 Noopur Gupta 2020-08-20 03:03:16 EDT

(In reply to Gautier de SAINT MARTIN LACAZE from comment #46)
> I hope I can do that today.

From 4.17 (2020-09) M3 milestone reminders:

- The "New and Noteworthy" entries are due on Wednesday evening:

Git repo: ssh://git.eclipse.org/gitroot/www.eclipse.org/eclipse/news.git

Gerrit: ssh://git.eclipse.org:29418/www.eclipse.org/eclipse/news.git

Live website: https://www.eclipse.org/eclipse/news/4.17/

Comment 48 Eclipse Genie 2020-08-20 15:28:04 EDT

New Gerrit change created: https://git.eclipse.org/r/c/www.eclipse.org/eclipse/news/+/168030

Comment 49 Eclipse Genie 2020-08-20 15:33:19 EDT

Gerrit change https://git.eclipse.org/r/c/www.eclipse.org/eclipse/news/+/168030 was merged to [master].
Commit: http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/?id=143691084293f0d7e75fe57af480236bb4410764

Comment 50 Jeff Johnston 2020-08-20 15:33:39 EDT

(In reply to Noopur Gupta from comment #47)
> (In reply to Gautier de SAINT MARTIN LACAZE from comment #46)
> > I hope I can do that today.
> 
> From 4.17 (2020-09) M3 milestone reminders:
> 
> - The "New and Noteworthy" entries are due on Wednesday evening:
> 
> Git repo: ssh://git.eclipse.org/gitroot/www.eclipse.org/eclipse/news.git
> 
> Gerrit: ssh://git.eclipse.org:29418/www.eclipse.org/eclipse/news.git
> 
> Live website: https://www.eclipse.org/eclipse/news/4.17/

I added an entry.

Comment 51 Jeff Johnston 2020-08-20 15:46:41 EDT

Verified for 4.17M3 using I20200818-0900

Comment 52 Eclipse Genie 2020-11-12 05:00:11 EST

New Gerrit change created: https://git.eclipse.org/r/c/jdt/eclipse.jdt.ui/+/172138

Comment 53 Eclipse Genie 2020-11-12 05:01:07 EST

Gerrit change https://git.eclipse.org/r/c/jdt/eclipse.jdt.ui/+/172138 was merged to [master].
Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=dd29d680137bb38e1720458416bd09b756b9208f