780892 - Split testFocusAndSearch.js into two test modules

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect, P2)

Description

[Maniac Vlad Florin (:vladmaniac)]

Right now 

/testSearch/testFocusAndSearch.js has two test functions and we need to split those into separate modules 

Please see https://bugzilla.mozilla.org/show_bug.cgi?id=761984#c37 for an official decision about this

Comment 1

[Maniac Vlad Florin (:vladmaniac)]

This will be addressed once the dependency has landed

Comment 2

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.0

Splitting testFocusAndSearch into two new modules: 

* testClickAndSearch.js
* testShortcutAndSearch.js

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 651317 [details] [diff] [review]
patch v1.0

>-[testFocusAndSearch.js]
>+[testClickAndSearch.js]
> disabled = Bug 761984 - Failure in testFocusAndSearch | Disconnect Error: Application unexpectedly closed

This line should have been removed from the file on bug 761984. Please post a follow-up there.

>+[testShortcutAndSearch.js]

I would better bundle them together by using testSearchVia(Focus|Shortcut).

Comment 4

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.1

* renamed test modules
* updated manifest

Comment 5

[Maniac Vlad Florin (:vladmaniac)]

This will not apply cleanly on mozilla-esr10, so if v1.1 is a r+, I will immediately post the patch for Firefox 10

Comment 6

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/da030c8c7502

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Dave, I assume you missed to set the r+ on the patch, right?

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

It looks like that restoreDefaultEngines doesn't work as expected. The shortcut test is failing now:

http://mozmill-ci.blargon7.com/#/functional/report/d87d47fd1034f072b9bece6ee6402324

Backed-out: http://hg.mozilla.org/qa/mozmill-tests/rev/0a9a654e62bd

Comment 9

[Dave Hunt [:davehunt] [he/him] ⌚BST]

I ran both tests, but not sequentially. A full testrun should be run before submitting a patch.

Comment 10

[Maniac Vlad Florin (:vladmaniac)]

We can always add a search engine with a different name in the shortcut test, and I think would be a decent band aid. 
Furthermore, we need a new bug to address restoreDefaultEngines problem

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

We want a fix not a workaround. Means we have to get restoreDefaultEngines fixed/enhanced before we can push this patch again. Please file a bug so it can be addressed.

Comment 12

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Dave Hunt (:davehunt) from comment #9)
> I ran both tests, but not sequentially. A full testrun should be run before
> submitting a patch.

A testrun was run of course, but this fails only when both tests are ran one after each other. 

Passing testrun: 
http://mozmill-crowd.blargon7.com/#/functional/report/d87d47fd1034f072b9bece6ee644df57

Failing testrun: 
http://mozmill-crowd.blargon7.com/#/functional/report/d87d47fd1034f072b9bece6ee644d4dc

Comment 13

[Dave Hunt [:davehunt] [he/him] ⌚BST]

The passing testrun linked in comment 12 is for restart tests. I doubt the non-restart testrun would ever pass as one test will always run after the other, and it appears that the test fails due to an artefact that is not cleaned up.

Comment 14

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Dave Hunt (:davehunt) from comment #13)
> The passing testrun linked in comment 12 is for restart tests. I doubt the
> non-restart testrun would ever pass as one test will always run after the
> other, and it appears that the test fails due to an artefact that is not
> cleaned up.

Ouch, wrong link - meant http://mozmill-crowd.blargon7.com/#/functional/report/d87d47fd1034f072b9bece6ee644b9ce

Comment 15

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Maniac Vlad Florin (:vladmaniac) from comment #14)
> Ouch, wrong link - meant
> http://mozmill-crowd.blargon7.com/#/functional/report/
> d87d47fd1034f072b9bece6ee644b9ce

That testrun does not include the separate tests testSearchViaFocus.js and testSearchViaShortcut.js, so the patch was likely not applied.

Comment 16

[Andrei Eftimie]

I've run some testruns with Vlad's original code ( attachment 651647 [details] [diff] [review] ), and had no Failures.

Here are the testruns:
20.0a1 us - Passed
http://mozmill-crowd.blargon7.com/#/functional/report/352218d7e3125c857fc1d37167fc8a02

20.0a1 fr - OS X - Passed
http://mozmill-crowd.blargon7.com/#/functional/report/a11aa7b15f4e571fd6fe21a2db03530a

20.0a1 fr - Win XP - Passed
http://mozmill-crowd.blargon7.com/#/functional/report/a11aa7b15f4e571fd6fe21a2db0347c8

I've also tested the problematic method restoreDefaultEngines and it appears to be working correctly 
(the Engine installed in setupModule is correctly uninstalled in teardown  )

Please advise on how to proceed.
Given no new failures, I would propose to reintegrate the patch.

Comment 17

[Andreea Matei [:AndreeaMatei]]

Andrei, the restoreDefaultEngines() was fixed in bug 783197.
After Henrik or Dave's commit you need to proceed by checking the other branches as well.

Comment 18

[Andrei Eftimie]

I have now ran the tests on all channels.

Aurora - 19.0a2 - Passed
http://mozmill-crowd.blargon7.com/#/functional/report/a11aa7b15f4e571fd6fe21a2db0fb307

Espre - 10.0.11esrpre - Passed
http://mozmill-crowd.blargon7.com/#/functional/report/a11aa7b15f4e571fd6fe21a2db0fda60

Espre - 17.0.1esrpre - Passed
http://mozmill-crowd.blargon7.com/#/functional/report/a11aa7b15f4e571fd6fe21a2db0fe506

Release - 17.0.1 - Tests Passed
http://mozmill-crowd.blargon7.com/#/functional/report/a11aa7b15f4e571fd6fe21a2db0ffc63
-- 2 Failures - seem to be related to bug 789987

Beta - 18.0 - Passed
http://mozmill-crowd.blargon7.com/#/functional/report/a11aa7b15f4e571fd6fe21a2db100cbf

bug 783197 appears to have fixed any problems regarding problems we've had here. Thanks Andreea

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Andrei Eftimie from comment #16)
> Please advise on how to proceed.
> Given no new failures, I would propose to reintegrate the patch.

That's what should be done if we want to mark it as fixed. Thanks for the investigation.

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 651647 [details] [diff] [review]
patch v1.1

The patch doesn't apply anymore. Please update and upload a new version.

Comment 21

[Andrei Eftimie]

Attachment: patch v1.2

Here is an updated patch.
Also ran a test to be sure everything is still good:
http://mozmill-crowd.blargon7.com/#/functional/report/a11aa7b15f4e571fd6fe21a2db279e5e

Comment 22

[Andrei Eftimie]

Attachment: patch v1.3

As per :AlexLakatos recommendation I have:
- removed the commented Litmus tests
- renamed the original file into one of the new ones, and newly created just 1 file

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 691816 [details] [diff] [review]
patch v1.3

Review of attachment 691816 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine. There are only two white-space issues which need to be fixed. With that updated r=me.

::: tests/functional/testSearch/testFocusAndSearch.js
@@ +7,3 @@
>  
>  const LOCAL_TEST_FOLDER = collector.addHttpResource('../../../data/');
> +const LOCAL_TEST_PAGE = LOCAL_TEST_FOLDER + 

nit: trialing whitespace.

::: tests/functional/testSearch/testSearchViaShortcut.js
@@ +5,5 @@
> +// Include necessary modules
> +var search = require("../../../lib/search");
> +
> +const LOCAL_TEST_FOLDER = collector.addHttpResource('../../../data/');
> +const LOCAL_TEST_PAGE = LOCAL_TEST_FOLDER + 

nit: trailing whitespace.

Comment 24

[Andrei Eftimie]

Attachment: patch v1.4

Comment 25

[Andreea Matei [:AndreeaMatei]]

Andrei, please add a blank line at the end of each test as the style guide requires and update the commit message with "r=hskupin, r=dhunt, r=amatei" - these are the names we are using here. 
Thanks

Comment 26

[Andrei Eftimie]

Attachment: Patch v1.5

- updated revier list to correct names
- added empty newline at the end of each file

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 692237 [details] [diff] [review]
Patch v1.5

Review of attachment 692237 [details] [diff] [review]:
-----------------------------------------------------------------

Just a note in general. Please request review from Andreea, Dave, and myself. That way it can be faster processed in the future.

Comment 28

[Henrik Skupin [:whimboo][⌚️UTC+2]]

http://hg.mozilla.org/qa/mozmill-tests/rev/584709b010d2 (default)

Please check tomorrows results and if the patch applies cleanly on the other branches, so we can get it backported.

Comment 29

[Andrei Eftimie]

All tests have passed:

Linux: http://mozmill-ci.blargon7.com/#/functional/report/a11aa7b15f4e571fd6fe21a2db83e6e6
OS X: http://mozmill-ci.blargon7.com/#/functional/report/a11aa7b15f4e571fd6fe21a2db7fb496
Win: http://mozmill-ci.blargon7.com/#/functional/report/a11aa7b15f4e571fd6fe21a2db834054

Can be marked as resolved

Comment 30

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Andrei Eftimie from comment #29)
> Can be marked as resolved

No, please check my last comment. We need backports.

Comment 31

[Andrei Eftimie]

Attachment: Patch v1.5 espre10

Patch for the mozilla-esr10 branch.

attachment 692237 [details] [diff] [review] applies cleanly to all other branches

Comment 32

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Transplanted as:
http://hg.mozilla.org/qa/mozmill-tests/rev/ba1049f1b77f (mozilla-aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/2651d7667541 (mozilla-beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/c2deb1b4d242 (mozilla-release)
http://hg.mozilla.org/qa/mozmill-tests/rev/c0126f6af170 (mozilla-esr17)

Comment 33

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 693847 [details] [diff] [review]
Patch v1.5 espre10

Review of attachment 693847 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/dee4259338d7

Comment 34

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Looks like we are done here. Marking as fixed.