1165556 - Update JavascriptTests implementing do_register_cleanup() callbacks

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Testing, defect)

Description

[Mark Capella [:capella]]

It seems that do_register_cleanup() callbacks [0] in our JavascriptTest (and possibly UITest) derived tests aren't being performed.

They're not triggered in robocop_head.js for tests other than child processes (those defined as run_test_in_child()), which we don't do. (Brief irc convo [1])

At the moment, uncompleted cleanup shouldn't be an issue as test processes don't run into each other, but we should remove those methods [2] [3] [4], and modify the consumers as appropriate. (cc:nalexander here in case this over-reaches).


[0] http://mxr.mozilla.org/mozilla-central/search?string=do_register_cleanup&case=1&find=mobile%2Fandroid&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

[1] http://logs.glob.uno/?c=mozilla%23mobile&s=15+May+2015&e=16+May+2015#c464316

[2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/tests/browser/robocop/robocop_head.js?mark=1020-1020#1008
[3] http://mxr.mozilla.org/mozilla-central/source/mobile/android/tests/browser/robocop/robocop_head.js?mark=312-312#311
[4] http://mxr.mozilla.org/mozilla-central/source/mobile/android/tests/browser/robocop/robocop_head.js?mark=894-894#887

Comment 1

[Mark Capella [:capella]]

Attachment: bug1165556_stub_register_cleanup.diff

Stub out main method |do_register_cleanup()|, and related but unused method that invokes it.

Comment 2

[Mark Capella [:capella]]

push to TRY all tests, prove target method is unused.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=55729c856385

Comment 3

[Mark Capella [:capella]]

Attachment: bug1165556_remove_run_test.diff

Remove method |run_test_in_child()|, (unused as in original report) and related but unused method that invokes it.

Comment 4

[Mark Capella [:capella]]

Attachment: bug1165556_remove_execute_test.diff

Remove method |_execute_test()|, (unused as in original report) and related but unused methods that are invoked.

Comment 5

[Mark Capella [:capella]]

Attachment: TEST_FAIL.diff

FYI ... DEBUGGING ONLY ...

Adds quick check for proper behaviour of known fails,
two todo tests,
and tags all attempts to invoke the now deprecated/stubbed method do_register_cleanup()

Comment 6

[Mark Capella [:capella]]

Push to TRY all tests with all obsolete code removed.

All random orange annotated and repushed until green / clean.
DEBUG test failures annotated.
Comparison of test results before / after shows no unexpected behaviour.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d47bd00cf27

Comment 7

[Mark Capella [:capella]]

At this point I think we could review/approved/move these patches as "Phase Initial".


"Phase Optional" could be further review of |robocop_head.js| for:
   1) Obsolete bits I may have overlooked related to this patch.
   --and--
   2) Un-Related unused bits we can opportunistically remove.

I can live without this or spin it off to another patch.


"Phase Main" will then be modifying the tests that invoke the unused callback method.

"Phase Final" will be simply removing the stubbed out |do_register_cleanup()| method.

Comment 8

[:Margaret Leibovic]

Thanks for picking this up!

nalexander is PTO until 7/28, so he won't be able to get to these for a while. However, gbrown or myself should be able to help with the reviews, if you want to redirect.

Comment 9

[Mark Capella [:capella]]

Thanks margaret! I noticed his absence on channel today and actually wondered :)

It'd be fine if you can steal direction here, and of course gbrown comments are welcome in this area.

I'll avoid updating the review? fields on the patches to save channel spam.

Comment 10

[:Margaret Leibovic]

(In reply to Mark Capella [:capella] from comment #9)
> Thanks margaret! I noticed his absence on channel today and actually
> wondered :)
> 
> It'd be fine if you can steal direction here, and of course gbrown comments
> are welcome in this area.

Never worry about bug spam!

I'll just NI myself to remember to look at this. If it's not in a queue somewhere, I'll forget :)

Comment 11

[Mark Capella [:capella]]

Comment on attachment 8635116 [details] [diff] [review]
bug1165556_stub_register_cleanup.diff

clearing review ... will replace with refined patchs

Comment 12

[Mark Capella [:capella]]

Comment on attachment 8635118 [details] [diff] [review]
bug1165556_remove_run_test.diff

clearing review ... will replace with refined patchs

Comment 13

[Mark Capella [:capella]]

Comment on attachment 8635121 [details] [diff] [review]
bug1165556_remove_execute_test.diff

clearing review ... will replace with refined patchs

Comment 14

[Mark Capella [:capella]]

Comment on attachment 8635116 [details] [diff] [review]
bug1165556_stub_register_cleanup.diff

No change from original version

Comment 15

[Mark Capella [:capella]]

Comment on attachment 8635118 [details] [diff] [review]
bug1165556_remove_run_test.diff

No change from original version

Comment 16

[Mark Capella [:capella]]

Attachment: bug1165556_remove_execute_test.diff

Removed an additional method I overlooked from related to this patch.

Comment 17

[Mark Capella [:capella]]

Attachment: bug1165556_update_register_cleanup.diff

Final patch, first post ... correctly implements original logic, and allows the tests to start actually executing the callbacks.

Note the change to |testHistoryService.js| ... Failed during my first try push, further proving we weren't doing callbacks previously.

Comment 18

[Mark Capella [:capella]]

so ... much ... green ...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c68a75b52c98

Comment 19

[:Margaret Leibovic]

Comment on attachment 8635116 [details] [diff] [review]
bug1165556_stub_register_cleanup.diff

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

::: mobile/android/tests/browser/robocop/robocop_head.js
@@ +887,5 @@
> +// XXX UNUSED - Registers a function that will run when the test harness
> +// is done running all tests.
> +//
> +function do_register_cleanup(unused) {
> +  // stubbed out until time to remove.

If nalexander and gbrown are cool with removing this, then I'm okay with it. I definitely believe we either need to remove it or fix it if it's going to be here.

Comment 20

[:Margaret Leibovic]

Comment on attachment 8635118 [details] [diff] [review]
bug1165556_remove_run_test.diff

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

I don't feel confident reviewing this, redirecting to gbrown.

(If I were to review this, it would just be a rubber stamp.)

Comment 21

[:Margaret Leibovic]

Comment on attachment 8635765 [details] [diff] [review]
bug1165556_remove_execute_test.diff

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

(Same story here)

Comment 22

[:Margaret Leibovic]

Comment on attachment 8635766 [details] [diff] [review]
bug1165556_update_register_cleanup.diff

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

::: mobile/android/tests/browser/robocop/testHistoryService.js
@@ +72,1 @@
>    });

So are we going to start encouraging people to actually clean up after their tests? I feel like this pattern is borrowed from desktop, where tests run sequentially without restarting the browser, so spurious tabs can cause unexpected failures.

I don't think cleanup functions like this actually serve a purpose in our test environment. Or is the long term plan to be able to run tests sequentially without creating new profiles between test runs?

I just want to make sure that we're implementing things that serve a purpose. Also, right now we're inconsistent in our use of do_register_cleanup, so I bet there are tests in the tree that don't even try to clean up after themselves.

Comment 23

[Mark Capella [:capella]]

As it is, we can see:

a) we're not currently running the cleanup callbacks that the original devs thought they had implemented.
b) we have cruft in robocop_head.js to remove either way (patches 2-3).

The current attached patches (patches 1-4) would properly implement what was thought to be the case all along, ie: cleanup functions that work.

What you suggest is to remove all code (ala patches 1-3), and add a final patch to complete that by removing the callback invocations in each test, and we're done.

Either approach will be fine with me :-) I'm happy to provide what you and gbrown may decide.

Comment 24

[Geoff Brown [:gbrown]]

(In reply to :Margaret Leibovic from comment #22)
> So are we going to start encouraging people to actually clean up after their
> tests? I feel like this pattern is borrowed from desktop, where tests run
> sequentially without restarting the browser, so spurious tabs can cause
> unexpected failures.
> 
> I don't think cleanup functions like this actually serve a purpose in our
> test environment. Or is the long term plan to be able to run tests
> sequentially without creating new profiles between test runs?
> 
> I just want to make sure that we're implementing things that serve a
> purpose. Also, right now we're inconsistent in our use of
> do_register_cleanup, so I bet there are tests in the tree that don't even
> try to clean up after themselves.

I mostly agree: In general, it should be unnecessary for robocop tests to cleanup, and if they don't need to, then they should not (to keep the test short and tidy if nothing else).

On the other hand...consider bug 1184186, where I aspire to migrate many Robocop JavascriptTests to mochitest-chrome. In that bug, I'm finding it quite handy to s/do_register_cleanup/SimpleTest.registerCleanupFunction/ -- and in those cases, cleanup is important, because the browser is not killed between tests and continues using the same profile. I hope that will be a one-time exercise, but I suppose other tests might one day be migrated to other test environments where cleanup is necessary. So in that light, I think there is some (minimal) value in having a way for a test to declare cleanup code, even if the cleanup is unnecessary.

Comment 25

[:Margaret Leibovic]

(In reply to Geoff Brown [:gbrown] (pto July 22 - 27) from comment #24)
> (In reply to :Margaret Leibovic from comment #22)
> > So are we going to start encouraging people to actually clean up after their
> > tests? I feel like this pattern is borrowed from desktop, where tests run
> > sequentially without restarting the browser, so spurious tabs can cause
> > unexpected failures.
> > 
> > I don't think cleanup functions like this actually serve a purpose in our
> > test environment. Or is the long term plan to be able to run tests
> > sequentially without creating new profiles between test runs?
> > 
> > I just want to make sure that we're implementing things that serve a
> > purpose. Also, right now we're inconsistent in our use of
> > do_register_cleanup, so I bet there are tests in the tree that don't even
> > try to clean up after themselves.
> 
> I mostly agree: In general, it should be unnecessary for robocop tests to
> cleanup, and if they don't need to, then they should not (to keep the test
> short and tidy if nothing else).
> 
> On the other hand...consider bug 1184186, where I aspire to migrate many
> Robocop JavascriptTests to mochitest-chrome. In that bug, I'm finding it
> quite handy to s/do_register_cleanup/SimpleTest.registerCleanupFunction/ --
> and in those cases, cleanup is important, because the browser is not killed
> between tests and continues using the same profile. I hope that will be a
> one-time exercise, but I suppose other tests might one day be migrated to
> other test environments where cleanup is necessary. So in that light, I
> think there is some (minimal) value in having a way for a test to declare
> cleanup code, even if the cleanup is unnecessary.

Thanks for the context, this is exactly the type of thing I was wondering about. In this case, I think it does make sense to implement the cleanup capability.

After we land these patches, we should also make a post to mobile-firefox-dev explaining the situation, so that everyone understands what's going on when they attempt to write these cleanup functions.

Comment 26

[Geoff Brown [:gbrown]]

Comment on attachment 8635765 [details] [diff] [review]
bug1165556_remove_execute_test.diff

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

Nice cleanup!

Comment 27

[Mark Capella [:capella]]

Thanks! So I'll stay on plan (these four patches), after margarets final approval :-)

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/a6e752520331
https://hg.mozilla.org/integration/fx-team/rev/cf3552e4a833
https://hg.mozilla.org/integration/fx-team/rev/d48fbf54c553
https://hg.mozilla.org/integration/fx-team/rev/5fc3405af99d

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a6e752520331
https://hg.mozilla.org/mozilla-central/rev/cf3552e4a833
https://hg.mozilla.org/mozilla-central/rev/d48fbf54c553
https://hg.mozilla.org/mozilla-central/rev/5fc3405af99d