Bug 499387 - Clean up, Speed up and document Browser test Suite
Summary: Clean up, Speed up and document Browser test Suite
Status: CLOSED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: SWT (show other bugs)
Version: 4.5   Edit
Hardware: PC All
: P3 enhancement (vote)
Target Milestone: 4.7   Edit
Assignee: Leo Ufimtsev CLA
QA Contact:
URL:
Whiteboard:
Keywords: test
Depends on:
Blocks: Webkit2Port4.7
  Show dependency tree
 
Reported: 2016-08-08 18:49 EDT by Leo Ufimtsev CLA
Modified: 2017-05-17 14:46 EDT (History)
3 users (show)

See Also:


Attachments
Before/After listeners tidy up (271.11 KB, image/png)
2017-02-06 15:12 EST, Leo Ufimtsev CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Leo Ufimtsev CLA 2016-08-08 18:49:16 EDT
Some of the tests in BrowserSuite are uneccessarily slow. Ex a test can pass but it will still do an idle loop. 
This makes testing very tedious.

I will submit some patches to try and speed things up and keep an eye to make sure it won't break test servers.
Comment 1 Eclipse Genie CLA 2016-08-08 18:52:36 EDT
New Gerrit change created: https://git.eclipse.org/r/78650
Comment 3 Eric Williams CLA 2016-08-09 09:57:28 EDT
(In reply to Eclipse Genie from comment #2)
> Gerrit change https://git.eclipse.org/r/78650 was merged to [master].
> Commit:
> http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/
> ?id=87f893f36b719c566a01e02580f73c322405087f

In master, leaving bug open as per Leo's request.
Comment 4 Eclipse Genie CLA 2016-08-09 12:54:07 EDT
New Gerrit change created: https://git.eclipse.org/r/78707
Comment 6 Eric Williams CLA 2016-08-09 13:43:15 EDT
(In reply to Eclipse Genie from comment #5)
> Gerrit change https://git.eclipse.org/r/78707 was merged to [master].
> Commit:
> http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/
> ?id=21a37adf8fd9364135a61c4078af874351ef4735

In master now.
Comment 7 Eclipse Genie CLA 2016-08-09 13:54:31 EDT
New Gerrit change created: https://git.eclipse.org/r/78712
Comment 9 Eric Williams CLA 2016-08-09 13:58:04 EDT
(In reply to Eclipse Genie from comment #8)
> Gerrit change https://git.eclipse.org/r/78712 was merged to [master].
> Commit:
> http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/
> ?id=a5c5d6ba31bbcbb84ac01132ebfbc4638cde645f

In master now.
Comment 10 Eclipse Genie CLA 2016-08-09 14:04:55 EDT
New Gerrit change created: https://git.eclipse.org/r/78714
Comment 12 Leo Ufimtsev CLA 2016-08-09 14:13:52 EDT
the 10 second tests now run in 2-3 seconds. ~improvement.
I'll check the build logs tomorrow to confirm that updates didn't break build servers.
Comment 13 Leo Ufimtsev CLA 2016-08-10 12:14:57 EDT
I checked build logs today, browser test 7,8,9 passed fine after modifications.

Tests now run ~2x faster. I can't see a straight forward way to make other tests faster without sacrificing stability.
Comment 14 Leo Ufimtsev CLA 2016-10-04 16:22:14 EDT
Some work left to be done in Test_*_Browser.. Will be submitting a few patches.

Tests would also benifit from a bit of documentation and proper naming. (Browser1, Brower2 is not very readable).
Comment 15 Eclipse Genie CLA 2016-10-04 16:27:42 EDT
New Gerrit change created: https://git.eclipse.org/r/82474
Comment 17 Eclipse Genie CLA 2016-10-04 18:17:40 EDT
New Gerrit change created: https://git.eclipse.org/r/82478
Comment 19 Niraj Modi CLA 2016-10-06 03:09:39 EDT
Recent changes made as part of this bug has caused below test failure on Windows:
http://download.eclipse.org/eclipse/downloads/drops4/I20161005-1430/testresults/html/org.eclipse.swt.tests_ep47I-unit-win32_win32.win32.x86_8.0.html#AllBrowserTests

Can see this particular JUnit failing locally as well on Win7.
Comment 20 Leo Ufimtsev CLA 2016-10-06 10:11:23 EDT
(In reply to comment #19)
> Recent changes made as part of this bug has caused below test failure on
> Windows:
> http://download.eclipse.org/eclipse/downloads/drops4/I20161005-1430/testresults/html/org.eclipse.swt.tests_ep47I-unit-win32_win32.win32.x86_8.0.html#AllBrowserTests
> 
> Can see this particular JUnit failing locally as well on Win7.

Sorry for trouble.

Let me investigate. It might be because I switched the parent of browser.
Comment 21 Leo Ufimtsev CLA 2016-10-06 11:31:04 EDT
(In reply to Niraj Modi from comment #19)
> Recent changes made as part of this bug has caused below test failure on
> Windows:
> http://download.eclipse.org/eclipse/downloads/drops4/I20161005-1430/
> testresults/html/org.eclipse.swt.tests_ep47I-unit-win32_win32.win32.x86_8.0.
> html#AllBrowserTests
> 
> Can see this particular JUnit failing locally as well on Win7.

I think the following could have broken it on windows is:
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_browser_Browser.setUp() {
+ setWidget(browser);
}

I lack a Windows test setup. Can you please try to delete that line and see if the jUnit works after that?

The reason I added that line was that otherwise the browser test cases share the shell with an empty composite. It's also what other test classes do, ex:
org.eclipse.swt.tests.junit.Test_org_eclipse_swt_widgets_ToolBar.setUp()

Let me know. If that fixes the tests, I'll revert that line for now and try to look for another solution. 

Thank you for your time.
Comment 22 Niraj Modi CLA 2016-10-07 04:32:57 EDT
(In reply to Leo Ufimtsev from comment #21)
> (In reply to Niraj Modi from comment #19)
> > Recent changes made as part of this bug has caused below test failure on
> > Windows:
> > http://download.eclipse.org/eclipse/downloads/drops4/I20161005-1430/
> > testresults/html/org.eclipse.swt.tests_ep47I-unit-win32_win32.win32.x86_8.0.
> > html#AllBrowserTests
> > 
> > Can see this particular JUnit failing locally as well on Win7.
> 
> I think the following could have broken it on windows is:
> org.eclipse.swt.tests.junit.Test_org_eclipse_swt_browser_Browser.setUp() {
> + setWidget(browser);
> }
> 
> I lack a Windows test setup. Can you please try to delete that line and see
> if the jUnit works after that?
> 
> The reason I added that line was that otherwise the browser test cases share
> the shell with an empty composite. It's also what other test classes do, ex:
> org.eclipse.swt.tests.junit.Test_org_eclipse_swt_widgets_ToolBar.setUp()
> 
> Let me know. If that fixes the tests, I'll revert that line for now and try
> to look for another solution. 
> 
> Thank you for your time.

Yes, reverting the setWidget(browser) line fixes the problem.

Reason for this issue:
After debugging this realized that IE Browser doesn't have 0 children.
Minimum child count for IE Browser will be 1, which is unlike reset of the composite classes.

Please see: IE#create(Composite parent, int style) method, we add OleFrame instance while creating IE browser.
Comment 23 Eclipse Genie CLA 2016-10-07 11:01:07 EDT
New Gerrit change created: https://git.eclipse.org/r/82740
Comment 24 Leo Ufimtsev CLA 2016-10-07 11:02:46 EDT
(In reply to Eclipse Genie from comment #23)
> New Gerrit change created: https://git.eclipse.org/r/82740

@ niraj
I submitted a patch that reverts the behaviour for windows. (But maintains for other platforms). 
Please let me know if this patch is satisfactory or if you have an idea for another patch.

Thank you for your time.
Comment 26 Niraj Modi CLA 2016-10-07 12:56:32 EDT
(In reply to Leo Ufimtsev from comment #24)
> (In reply to Eclipse Genie from comment #23)
> > New Gerrit change created: https://git.eclipse.org/r/82740
> 
> @ niraj
> I submitted a patch that reverts the behaviour for windows. (But maintains
> for other platforms). 
> Please let me know if this patch is satisfactory or if you have an idea for
> another patch.
> 
> Thank you for your time.

Thanks Leo for the fix, have verified it on my Win7 setup, pushed to master.
Comment 27 Leo Ufimtsev CLA 2016-10-07 15:21:21 EDT
(In reply to comment #26)

> Thanks Leo for the fix, have verified it on my Win7 setup, pushed to master.

Nice. 

I added a test case to Browser. *Theoretically* should work across all platforms :-). See:
505591: Add test case for Browser.evaluate() to browser test suite.
https://bugs.eclipse.org/bugs/show_bug.cgi?id=505591
Comment 28 Leo Ufimtsev CLA 2017-02-06 15:06:13 EST
Re-opening to tidy up some Listener-related tests.
Comment 29 Eclipse Genie CLA 2017-02-06 15:06:58 EST
New Gerrit change created: https://git.eclipse.org/r/90451
Comment 30 Leo Ufimtsev CLA 2017-02-06 15:12:52 EST
Created attachment 266677 [details]
Before/After listeners tidy up

(In reply to Eclipse Genie from comment #29)
> New Gerrit change created: https://git.eclipse.org/r/90451

I was working on listeners and found it time consuming to find all test cases related to a particular listener. So I tidied things up.
Comment 31 Eclipse Genie CLA 2017-02-06 16:55:30 EST
New Gerrit change created: https://git.eclipse.org/r/90462
Comment 32 Leo Ufimtsev CLA 2017-02-06 17:00:57 EST
(In reply to Eclipse Genie from comment #31)
> New Gerrit change created: https://git.eclipse.org/r/90462

Refactoring duplicated code into lambdas.
Comment 33 Leo Ufimtsev CLA 2017-02-07 10:54:14 EST
I will break down the large commit into smaller once. To be continued in:
Bug 511849 - [Webkit] Refactor Browser Listener tests