Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 324658

Summary:

OpenSourceCommandHandler causes slowness and memory usage in Test Navigator

Product:

z_Archived

Reporter:

Paul Slauenwhite <paulslau>

Component:

TPTP

Assignee:

Bozier jerome <jerome.bozier>

Status:

CLOSED DUPLICATE

QA Contact:

Kathy Chan <kathy>

Severity:

major

Priority:

CC:

alexberns

Version:

unspecified

Flags:

paulslau: review-

Target Milestone:

---

Hardware:

All

OS:

All

Whiteboard:

adopter

Bug Depends on:

327681

Bug Blocks:

Attachments:

Description	Flags
patch	none

Description Paul Slauenwhite

2010-09-07 10:21:31 EDT

org.eclipse.hyades.test.tools.ui.java.internal.junit.handlers.OpenSourceCommandHandler tries to enable some action based on whether the selected node in test navigator has an associated JUnit java source file. The problem is that it attempts to load the entire test into memory every time this decision needs to be made. For huge tests it becomes a really big slowdown in performance.

Will it be possible to optimize this behavior in some way, perhaps check once and create a true/false markers on both files (the testsuite and associated Java, or testsuite/nothing if there is no java).

Comment 1 Paul Slauenwhite

2010-09-07 10:23:23 EDT

Unfortunately, resolving the test suite from the proxy node is necessary to resolve the associated test class, which is in the test suite/case model.  We may be able change the behaviour of this handler by always enabling it for test suite/case proxy nodes and then once executed, display an error dialog if the associated test class cannot be resolved (e.g. a test class is not defined in the test suite).

Requires a sizing.

Comment 2 Bozier jerome

2010-10-15 09:56:36 EDT

Created attachment 180957 [details]
patch

this patch fixes the problem on the following way :
. menu is always enabled on junit test case/test suite
. check is only performed when executing the command
. if check fail, an error message appear

because of bugzilla 327681, i am not able to create a test case to reproduce the problem (menu does no longer appear).
to be able to reproduce it, i had to use a tptp 4.7.0 and only import hyades.test.tools.ui (hyades.test.tool.junit.plugin disable the menu entry when imported)

Comment 3 Bozier jerome

2010-10-15 09:57:46 EDT

Paul, could you review this patch please ?

many thanks in advance

Comment 4 Paul Slauenwhite

2010-10-18 09:09:40 EDT

Patch reviewed with the following comments:

-I would tighten the constraints on enabling this menu.  I would check the proxy type and only enable for JUnit/JUnit Plug-in/URL suite/case proxy nodes, since this context menu will be incorrectly enabled but broken in the consuming product.

-We cannot create new messages in a maintenance release.  I would suggest logging the error condition and reusing WorkspaceResourceProviderService_SOURCE_FILE_DNE in /org.eclipse.hyades.test.core/src/org/eclipse/hyades/test/core/testservices/resources/messages.properties.

-Defect 327681 should be fixed before delivering the fix for this for testing purposes.

Comment 5 Bozier jerome

2010-10-19 07:37:26 EDT

(In reply to comment #4)
> Patch reviewed with the following comments:
> 
> -I would tighten the constraints on enabling this menu.  I would check the
> proxy type and only enable for JUnit/JUnit Plug-in/URL suite/case proxy nodes,
> since this context menu will be incorrectly enabled but broken in the consuming
> product.
> 
this constraint is done in the plugin.xml file : we already check if the selected element is instance of :
. JUnitTestSuiteProxyNode
. JUnitTestCaseProxyNode
. junit HTTPTestSuiteProxyNode
. junit HTTPTestCaseProxyNode
. JUnitPluginTestSuiteProxyNode
. JUnitPluginTestCaseProxyNode
check at selection time is so useless outside of checking if source exists (and we remove this part)

> -We cannot create new messages in a maintenance release.  I would suggest
> logging the error condition and reusing
> WorkspaceResourceProviderService_SOURCE_FILE_DNE in
> /org.eclipse.hyades.test.core/src/org/eclipse/hyades/test/core/testservices/resources/messages.properties.
on it
 
> -Defect 327681 should be fixed before delivering the fix for this for testing
> purposes.
done

new patch incoming soon with fixed messages

Comment 6 Paul Slauenwhite

2010-10-19 09:08:42 EDT

(In reply to comment #5)
> (In reply to comment #4)
> > Patch reviewed with the following comments:
> > 
> > -I would tighten the constraints on enabling this menu.  I would check the
> > proxy type and only enable for JUnit/JUnit Plug-in/URL suite/case proxy nodes,
> > since this context menu will be incorrectly enabled but broken in the consuming
> > product.
> > 
> this constraint is done in the plugin.xml file : we already check if the
> selected element is instance of :
> . JUnitTestSuiteProxyNode
> . JUnitTestCaseProxyNode
> . junit HTTPTestSuiteProxyNode
> . junit HTTPTestCaseProxyNode
> . JUnitPluginTestSuiteProxyNode
> . JUnitPluginTestCaseProxyNode
> check at selection time is so useless outside of checking if source exists (and
> we remove this part)

You're right.  Thanks.

> > -We cannot create new messages in a maintenance release.  I would suggest
> > logging the error condition and reusing
> > WorkspaceResourceProviderService_SOURCE_FILE_DNE in
> > /org.eclipse.hyades.test.core/src/org/eclipse/hyades/test/core/testservices/resources/messages.properties.
> on it

Thanks.

> > -Defect 327681 should be fixed before delivering the fix for this for testing
> > purposes.
> done
> 
> new patch incoming soon with fixed messages

Great!

Comment 7 Bozier jerome

2010-10-25 07:34:41 EDT

fix provided in 327681 patch (this part of code have been refactored to fix both problems)

*** This bug has been marked as a duplicate of bug 327681 ***