Closed Bug 527444 Opened 10 years ago Closed 3 years ago

Stop using tail_ files in xpcshell: use do_register_cleanup() instead, in Core and Toolkit

Categories

(Core :: General, defect, trivial)

defect
Not set
trivial

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: sgautherie, Assigned: standard8, Mentored)

References

()

Details

(Whiteboard: [patchlove])

Attachments

(1 file, 10 obsolete files)

59 bytes, text/x-review-board-request
Paolo
: review+
Details
[
/modules/libjar/zipwriter/test/unit/tail_zipwriter.js
/uriloader/exthandler/tests/unit/tail_handlerService.js

/toolkit/components/contentprefs/tests/unit/tail_contentPrefs.js
/toolkit/components/url-classifier/tests/unit/tail_urlclassifier.js
/toolkit/mozapps/extensions/test/unit/tail_extensionmanager.js
]

All these are (near) one-liners.
So it should be trivial to move this call to the corresponding head files.
Flags: in-testsuite-
Whiteboard: [good first bug]
Depends on: 502909
(In reply to comment #0)
> [
> /modules/libjar/zipwriter/test/unit/tail_zipwriter.js
> /uriloader/exthandler/tests/unit/tail_handlerService.js
> 
> /toolkit/components/contentprefs/tests/unit/tail_contentPrefs.js
> /toolkit/components/url-classifier/tests/unit/tail_urlclassifier.js
> /toolkit/mozapps/extensions/test/unit/tail_extensionmanager.js
> ]
> 
> All these are (near) one-liners.
> So it should be trivial to move this call to the corresponding head files.

Do you mean, for instance, move the contents of:

/toolkit/components/contentprefs/tests/unit/tail_contentPrefs.js

to

/toolkit/components/contentprefs/tests/unit/head_contentPrefs.js

?
(In reply to comment #1)
> Do you mean, for instance, move the contents of:
> /toolkit/components/contentprefs/tests/unit/tail_contentPrefs.js
> to
> /toolkit/components/contentprefs/tests/unit/head_contentPrefs.js
> ?

Yes.
In addition, the moved code should be put in a function, which should be do_register_cleanup()'ed.
Attached patch zipwriter diff file (obsolete) — Splinter Review
/toolkit/components/url-classifier/tests/unit/tail_urlclassifier.js
contains this code:
cleanUp();

And, /toolkit/components/url-classifier/tests/unit/head_urlclassifier.js
includes a call to cleanUp() at the bottom (line 307) 

This need a refactor too?
Attached patch Multiple files diff (obsolete) — Splinter Review
Attachment #499947 - Attachment is obsolete: true
Comment on attachment 499950 [details] [diff] [review]
Multiple files diff

>+function do_register_cleanup(){

You probably rather need:

+function do_register_cleanup() {
+  function () {

>+    try {
>+      zipW.close();
>+    }
>+    catch (e) {
>+      // Just ignore a failure here and attempt to delete the file anyway.
>+    }
>+
>+    if (tmpFile.exists())
>+      tmpFile.remove(true);    

+  }

>+}

And you need to remove the code from where it currently is too.
Attachment #499950 - Flags: review-
Assignee: nobody → martincerdeira
Status: NEW → ASSIGNED
(In reply to comment #4)
> /toolkit/components/url-classifier/tests/unit/tail_urlclassifier.js
> 
> This need a refactor too?

Yes, one call is executed before the test, the other one after the test.
(In reply to comment #6)
> You probably rather need:
> 
> +function do_register_cleanup() {
> +  function () {

+do_register_cleanup(
+  function () {

> >+    try {
> >+      zipW.close();
> >+    }
> >+    catch (e) {
> >+      // Just ignore a failure here and attempt to delete the file anyway.
> >+    }
> >+
> >+    if (tmpFile.exists())
> >+      tmpFile.remove(true);    
> 
> +  }
> 
> >+}

+);

actually ;->
Martin, ping for update.
Whiteboard: [good first bug] → [patchlove] [good first bug][mentor=sgautherie][lang=js]
Martin, could you confirm that you're still working on this bug?
Flags: needinfo?(martincerdeira)
(In reply to Josh Matthews [:jdm] from comment #10)
> Martin, could you confirm that you're still working on this bug?

Josh, Im not working on this.
Flags: needinfo?(martincerdeira)
Assignee: martincerdeira → nobody
Status: ASSIGNED → NEW
Hi, can I work on this bug?
Assignee: nobody → rahid.robin
Attached patch stop_using_tail_files.patch (v1) (obsolete) — Splinter Review
Attachment #8360824 - Flags: review?(sgautherie.bz)
Attachment #8360824 - Flags: feedback?(josh)
Attached patch stop_using_tail_files.patch (v2) (obsolete) — Splinter Review
Attachment #8360824 - Attachment is obsolete: true
Attachment #8360824 - Flags: review?(sgautherie.bz)
Attachment #8360824 - Flags: feedback?(josh)
Attachment #8360833 - Flags: review?(sgautherie.bz)
Attachment #8360833 - Flags: feedback?(josh)
Comment on attachment 8360833 [details] [diff] [review]
stop_using_tail_files.patch (v2)

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

This looks really good Rahid! Sorry about the long delay. I have two concerns - have you the download manager tests and the app tests? Those are the changes that look slightly more concerning to me, and I want to ensure that the tests still pass correctly.

::: modules/libjar/zipwriter/test/unit/head_zipwriter.js
@@ +45,5 @@
>  
>  var zipW = new ZipWriter();
> +
> +do_register_cleanup(
> +	function () {

Move this onto the previous line.

@@ +55,5 @@
> +		}
> +
> +		if (tmpFile.exists())
> +		  tmpFile.remove(true);
> +	}

This is using tabs; please use spaces instead.

::: modules/libjar/zipwriter/test/unit/xpcshell.ini
@@ +1,3 @@
>  [DEFAULT]
>  head = head_zipwriter.js
> +tail = 

Remove the trailing space.

::: toolkit/components/contentprefs/tests/unit/head_contentPrefs.js
@@ +164,5 @@
>    prefBranch.setBoolPref("browser.preferences.content.log", true);
>  }
>  
> +do_register_cleanup(
> +  function () {

Previous line.

::: toolkit/components/contentprefs/tests/unit/xpcshell.ini
@@ +1,3 @@
>  [DEFAULT]
>  head = head_contentPrefs.js
> +tail = 

Trailing space.

::: toolkit/components/downloads/test/unit/head_download_manager.js
@@ +232,5 @@
>    return false;
> +}
> +
> +do_register_cleanup(
> +  function test_common_terminate()

Previous line.

@@ +233,5 @@
> +}
> +
> +do_register_cleanup(
> +  function test_common_terminate()
> +  {

Move this on the same line as the function name.

::: toolkit/components/downloads/test/unit/xpcshell.ini
@@ +1,3 @@
>  [DEFAULT]
>  head = head_download_manager.js
> +tail = 

Trailing space.

::: toolkit/components/url-classifier/tests/unit/head_urlclassifier.js
@@ +359,5 @@
>  };
>  
>  cleanUp();
> +
> +do_register_cleanup(

This can just be |do_register_cleanup(cleanUp);|

::: toolkit/components/url-classifier/tests/unit/xpcshell.ini
@@ +1,3 @@
>  [DEFAULT]
>  head = head_urlclassifier.js
> +tail = 

Trailing space.

::: toolkit/devtools/apps/tests/unit/head_apps.js
@@ +123,5 @@
>    Services.dirsvc.QueryInterface(Ci.nsIDirectoryService).registerProvider(provider);
>  }
>  
> +do_register_cleanup(
> +  function () {

Previous line.

::: toolkit/devtools/apps/tests/unit/xpcshell.ini
@@ +1,3 @@
>  [DEFAULT]
>  head = head_apps.js
> +tail = 

Trailing space.

::: toolkit/identity/tests/unit/head_identity.js
@@ +212,5 @@
>  // Switch debug messages on by default
>  Services.prefs.setBoolPref("toolkit.identity.debug", true);
> +
> +do_register_cleanup(
> +  function () {

Previous line.

::: toolkit/identity/tests/unit/xpcshell.ini
@@ +1,3 @@
>  [DEFAULT]
>  head = head_identity.js
> +tail = 

Trailing space.

::: uriloader/exthandler/tests/unit/head_handlerService.js
@@ +162,5 @@
>  
>  HandlerServiceTest.init();
> +
> +do_register_cleanup(
> +  function () {

Previous line.

::: uriloader/exthandler/tests/unit/xpcshell.ini
@@ +1,3 @@
>  [DEFAULT]
>  head = head_handlerService.js
> +tail = 

Trailing space.
Attachment #8360833 - Flags: feedback?(josh) → feedback+
Attachment #499950 - Attachment is obsolete: true
Comment on attachment 8360833 [details] [diff] [review]
stop_using_tail_files.patch (v2)

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

(Sorry for the 2,5 month delay.)
Patch looks mainly good :-)

::: modules/libjar/zipwriter/test/unit/head_zipwriter.js
@@ +45,5 @@
>  
>  var zipW = new ZipWriter();
> +
> +do_register_cleanup(
> +	function () {

In addition to comment 15, my preference would be to name the added function(s), like "function tail_zipwriter()", or any better name you can think of.

::: modules/libjar/zipwriter/test/unit/xpcshell.ini
@@ +1,3 @@
>  [DEFAULT]
>  head = head_zipwriter.js
> +tail = 

Guenuine question:
Is it even needed/wanted to keep an empty "tail ="? Or could it be removed?
Attachment #8360833 - Flags: review?(bugzillamozillaorg_serge_20140323) → feedback+
Attached patch stop_using_tail_files.patch (v3) (obsolete) — Splinter Review
This is updated patch based on previous review.
Attachment #8360833 - Attachment is obsolete: true
Attachment #8416058 - Flags: review?(bugzillamozillaorg_serge_20140323)
Attachment #8416058 - Flags: feedback?(josh)
Comment on attachment 8416058 [details] [diff] [review]
stop_using_tail_files.patch (v3)

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

In this patch, you missed to remove the tail files themselves: see your previous patch :-(
I found some (style) consistency nits...

::: modules/libjar/zipwriter/test/unit/head_zipwriter.js
@@ +42,5 @@
>  tmpFile.append("zipwriter-test.zip");
>  if (tmpFile.exists())
>    tmpFile.remove(true);
>  
>  var zipW = new ZipWriter();

Add a blank line before the added "do_register_cleanup(...". (Here and elsewhere.))

@@ +43,5 @@
>  if (tmpFile.exists())
>    tmpFile.remove(true);
>  
>  var zipW = new ZipWriter();
> +do_register_cleanup(function tail_zipwriter () {

No space before "()". (Here and elsewhere.)

@@ +44,5 @@
>    tmpFile.remove(true);
>  
>  var zipW = new ZipWriter();
> +do_register_cleanup(function tail_zipwriter () {
> +    try {

2-space indentations are enough. (Here and elsewhere.)
NB: I might not have minded the initial 4-space, but 2-space there too seems more consistent with (some) other files...

::: toolkit/components/downloads/test/unit/head_download_manager.js
@@ +230,5 @@
>      return true;
>    }
>    return false;
> +}
> +do_register_cleanup(function test_common_terminate() {

"tail_download_manager()", to be consistent?

::: toolkit/components/url-classifier/tests/unit/head_urlclassifier.js
@@ +361,5 @@
>      return (val >>> (32 - bits));
>    },
>  };
>  
> +do_register_cleanup(cleanUp);

No, this should be:

cleanUp(); // The existing one.

do_register_cleanup(cleanUp); // The one you move.

::: toolkit/devtools/apps/tests/unit/head_apps.js
@@ +119,5 @@
>    Services.dirsvc.QueryInterface(Ci.nsIDirectoryService).registerProvider(provider);
>  }
> +do_register_cleanup(function tail_apps () {
> +  if (gClient) {
> +    // Close the test remote connection before leaving this test

While here, add a ".". (Maybe elsewhere too.)

@@ +120,5 @@
>  }
> +do_register_cleanup(function tail_apps () {
> +  if (gClient) {
> +    // Close the test remote connection before leaving this test
> +    gClient.close(function () {

(Add a dumb function name here too.)

::: toolkit/identity/tests/unit/head_identity.js
@@ +246,5 @@
>  } catch(noPref) {}
>  Services.prefs.setBoolPref("identity.fxaccounts.enabled", true);
>  
>  // after execution, restore prefs
>  do_register_cleanup(function() {

"tail_identity()", to be consistent?
Attachment #8416058 - Flags: review?(bugzillamozillaorg_serge_20140323)
Attachment #8416058 - Flags: review-
Attachment #8416058 - Flags: feedback?(josh)
Hi, Rahid - Are you still working on this?
Flags: needinfo?(rahid.robin)
Mentor: bugzillamozillaorg_serge_20140323
Whiteboard: [patchlove] [good first bug][mentor=sgautherie][lang=js] → [patchlove] [good first bug][lang=js]
Can I work on this?
Flags: needinfo?(mhoye)
(In reply to Mike Hoye [:mhoye] from comment #19)
> Hi, Rahid - Are you still working on this?

(No answer for 5 months.)


(In reply to Akshendra Pratap Singh from comment #20)
> Can I work on this?

Sure.
Assignee: rahid.robin → akshendra521994
Status: NEW → ASSIGNED
Flags: needinfo?(rahid.robin)
Flags: needinfo?(mhoye)
Attachment #8515565 - Flags: feedback?(bugzillamozillaorg_serge_20140323)
review ping
Flags: needinfo?(bugzillamozillaorg_serge_20140323)
Comment on attachment 8515565 [details] [diff] [review]
527444_StopUsingTailFiles.patch (v1)

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

I'll look at this.
Attachment #8515565 - Flags: feedback?(bugzillamozillaorg_serge_20140323) → review?(josh)
Comment on attachment 8515565 [details] [diff] [review]
527444_StopUsingTailFiles.patch (v1)

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

This looks fine to me. I'm going to throw it at try and see if anything breaks!
Attachment #8515565 - Flags: review?(josh) → review+
Attachment #8416058 - Attachment is obsolete: true
Flags: needinfo?(bugzillamozillaorg_serge_20140323)
Comment on attachment 8515565 [details] [diff] [review]
527444_StopUsingTailFiles.patch (v1)

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

(Sorry for the one month delay.)

Good patch :-)
A few nits below.
Also, try and fix "\ No newline at end of file" in the modified files.

::: toolkit/components/url-classifier/tests/unit/head_urlclassifier.js
@@ +368,5 @@
>  
>  cleanUp();
> +
> +do_register_cleanup(function tail_ulrclassifier() {
> +  cleanUp();

(In reply to Josh Matthews [:jdm] from comment #15)
> This can just be |do_register_cleanup(cleanUp);|

Agreed.
Yet, fwiw, my preference would be to have this as in this patch, for "clarity" (as in not to confuse the 2 cleanUp() calls).

::: toolkit/devtools/apps/tests/unit/head_apps.js
@@ +130,5 @@
>  }
>  
> +do_register_cleanup(function tail_apps() {
> +  if (gClient) {
> +    // Close the test remote connection before leaving this test

(In reply to Serge Gautherie (:sgautherie) from comment #18)
> While here, add a ".". (Maybe elsewhere too.)

(At the end of the sentence.)

@@ +131,5 @@
>  
> +do_register_cleanup(function tail_apps() {
> +  if (gClient) {
> +    // Close the test remote connection before leaving this test
> +    gClient.close(function () {

(In reply to Serge Gautherie (:sgautherie) from comment #18)
> (Add a dumb function name here too.)

(Like "tail_apps_close" for example.)
Attachment #8515565 - Flags: feedback-
Attachment #8515565 - Attachment is obsolete: true
Attachment #8530879 - Flags: feedback?(bugzillamozillaorg_serge_20140323)
Comment on attachment 8530879 [details] [diff] [review]
527444_StopUsingTailFiles.patch (v2)

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

Good, though still a few nits.

::: toolkit/devtools/apps/tests/unit/head_apps.js
@@ +128,5 @@
>    };
>    Services.dirsvc.QueryInterface(Ci.nsIDirectoryService).registerProvider(provider);
>  }
>  
> +do_register_cleanup(function tail_apps_close() {

No, 'tail_apps' was fine here.

@@ +131,5 @@
>  
> +do_register_cleanup(function tail_apps_close() {
> +  if (gClient) {
> +    // Close the test remote connection before leaving this test.
> +    gClient.close(function () {

Add 'tail_apps_close' here.

::: toolkit/devtools/apps/tests/unit/xpcshell.ini
@@ +1,3 @@
>  [DEFAULT]
>  head = head_apps.js
> +tail =

Why are you readding this "empty" line?

::: toolkit/identity/tests/unit/head_identity.js
@@ +253,5 @@
>    Services.prefs.setBoolPref("identity.fxaccounts.enabled", initialPrefFXAValue);
>  });
>  
> +do_register_cleanup(function tail_identity() {
> +  // pre-emptively shut down to clear resources.

Nit: "Pre-emptively", while there.
Attachment #8530879 - Flags: feedback?(bugzillamozillaorg_serge_20140323) → feedback-
Attachment #8530879 - Attachment is obsolete: true
Attachment #8531217 - Flags: review?(bugzillamozillaorg_serge_20140323)
Attachment #8360824 - Attachment description: stop_using_tail_files.patch → stop_using_tail_files.patch (v1)
Attachment #8360833 - Attachment description: stop_using_tail_files.patch → stop_using_tail_files.patch (v2)
Attachment #8416058 - Attachment description: stop_using_tail_files.patch → stop_using_tail_files.patch (v3)
Attachment #8515565 - Attachment description: 527444_StopUsingTailFiles.patch → 527444_StopUsingTailFiles.patch (v1)
Attachment #8530879 - Attachment description: 527444_StopUsingTailFiles.patch → 527444_StopUsingTailFiles.patch (v2)
Attachment #8531217 - Attachment description: 527444_StopUsingTailFiles.patch → 527444_StopUsingTailFiles.patch (v3)
Comment on attachment 8531217 [details] [diff] [review]
527444_StopUsingTailFiles.patch (v3)

Looks good :-)

>Bug 527444: Stopped using tail_files in Core and Toolkit; r=sgautherie

Nit: more like
Bug 527444: use "do_register_cleanup(...)" calls instead of "tail_*.js" files in xpcshell, in Core and Toolkit; f=sgautherie r=josh.

>--- a/toolkit/components/downloads/test/unit/tail_download_manager.js
>+++ /dev/null
>@@ -1,24 +0,0 @@
>-/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */
>-/* vim: set ts=2 et sw=2 tw=80: */
>-/* This Source Code Form is subject to the terms of the Mozilla Public
>- * License, v. 2.0. If a copy of the MPL was not distributed with this
>- * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>-
>-/**
>- * Provides infrastructure for automated download components tests.
>- */
>-
>-"use strict";
>-
>-////////////////////////////////////////////////////////////////////////////////
>-//// Termination functions common to all tests

josh, can you (double) check whether any of these lines should be copied to the head file?
(Especially the "use strict" line.)
Attachment #8531217 - Flags: review?(josh)
Attachment #8531217 - Flags: review?(bugzillamozillaorg_serge_20140323)
Attachment #8531217 - Flags: feedback+
Comment on attachment 8531217 [details] [diff] [review]
527444_StopUsingTailFiles.patch (v3)

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

The head file doesn't contain `use strict`, but I don't think it's worth introducing at this point.
Attachment #8531217 - Flags: review?(josh) → review+
Attachment #8531217 - Attachment is obsolete: true
Attachment #8535621 - Flags: review+
Try run was green. Let's merge this!
Keywords: checkin-needed
(In reply to Wes Kocher (:KWierso) from comment #37)
> https://treeherder.mozilla.org/ui/logviewer.
> html#?job_id=4598518&repo=mozilla-inbound

{
12. '/tools/buildbot/bin/python scripts/scripts/android_emulator_unittest.py ...' warnings 41m 18s 24ms
2901 12:31:45 INFO - KeyError: 'tail'
2910 12:31:45 INFO - KeyError: 'tail'
2911 12:31:45 ERROR - No tests run or test summary not found
}

Is this because remotexpcshelltests.py would not support having no tail line in the manifest?
http://mxr.mozilla.org/mozilla-central/source/testing/xpcshell/remotexpcshelltests.py#81
99         return (list(sanitize_list(test['head'], 'head')),
100                 list(sanitize_list(test['tail'], 'tail')))

Unlike runxpcshelltests.py:
http://mxr.mozilla.org/mozilla-central/source/testing/xpcshell/runxpcshelltests.py#379
401         headlist = test_object['head'] if 'head' in test_object else ''
402         taillist = test_object['tail'] if 'tail' in test_object else ''
403         return (list(sanitize_list(headlist, 'head')),
404                 list(sanitize_list(taillist, 'tail')))

If so, we could leave dumb "tail=" lines, while the harness issue is fixed in a separate bug.
Flags: needinfo?(akshendra521994)
Comment on attachment 8535621 [details] [diff] [review]
527444_StopUsingTailFiles.patch (v4)

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

Hi, thanks for the work to help keeping our code base in good shape!

I've noticed an issue with the patch for the old "downloads" folder, that I described below.

Note that tests in this folder may be removed in the next few months, or maybe earlier, but in the meantime we should make sure we do the right thing.

::: toolkit/components/downloads/test/unit/head_download_manager.js
@@ +232,5 @@
>    return false;
>  }
> +
> +do_register_cleanup(function tail_download_manager() {
> +  add_task(function test_common_terminate() {

"add_task" has no effect during cleanup functions - in fact, the existing code was broken, as "add_task" does not work properly inside "tail.js" files as well.

The cleanup function, instead, should just return the promise. This ensures that we wait until the HTTP server has shut down before continuing.
Attachment #8535621 - Flags: feedback-
Flags: needinfo?(josh)
This addresses your previous feedback on the patch.
Attachment #8704833 - Flags: review?(paolo.mozmail)
Attachment #8535621 - Attachment is obsolete: true
Assignee: akshendra521994 → josh
Comment on attachment 8704833 [details] [diff] [review]
use "do_register_cleanup(...)" calls instead of "tail_*.js" files in xpcshell, in Core and Toolkit; f=sgautherie

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

Looks good to me with the changes below assuming a tryserver build succeeds. Thanks!

::: devtools/shared/apps/tests/unit/head_apps.js
@@ +134,5 @@
> +do_register_cleanup(function tail_apps() {
> +  if (gClient) {
> +    // Close the test remote connection before leaving this test.
> +    gClient.close(function tail_apps_close() {
> +      run_next_test();

Again, it seems like you want to return a Promise here. I'm not even sure that "run_next_test" did what it was supposed to in the tail file.

Note that if you use both do_register_cleanup and do_get_profile, the relative order of those calls is important for network-related operations. See <https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests#Environment> that I recently updated.

::: toolkit/components/downloads/test/unit/head_download_manager.js
@@ +177,5 @@
> +    // Stop the HTTP server.  We must do this inside a task in "tail.js" until the
> +    // xpcshell testing framework supports asynchronous termination functions.
> +    let deferred = Promise.defer();
> +    gHttpServer.stop(deferred.resolve);
> +    return deferred.promise;

Bonus points for using the "new Promise()" syntax, despite we're removing this code completely at some point :-)

::: uriloader/exthandler/tests/unit/head_handlerService.js
@@ +161,5 @@
>  };
>  
>  HandlerServiceTest.init();
> +
> +do_register_cleanup(function tail_handleService() {

nit: tail_handlerService
Attachment #8704833 - Flags: review?(paolo.mozmail) → review+
(In reply to :Paolo Amadini from comment #44)
> ::: devtools/shared/apps/tests/unit/head_apps.js
> @@ +134,5 @@
> > +do_register_cleanup(function tail_apps() {
> > +  if (gClient) {
> > +    // Close the test remote connection before leaving this test.
> > +    gClient.close(function tail_apps_close() {
> > +      run_next_test();
> 
> Again, it seems like you want to return a Promise here. I'm not even sure
> that "run_next_test" did what it was supposed to in the tail file.
> 
> Note that if you use both do_register_cleanup and do_get_profile, the
> relative order of those calls is important for network-related operations.
> See
> <https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-
> based_unit_tests#Environment> that I recently updated.

I originally attempted this change when updating this patch and encountered test failures.
(In reply to Josh Matthews [:jdm] from comment #45)
> I originally attempted this change when updating this patch and encountered
> test failures.

It may well be caused by the ordering issue, so it may be easy to fix. If it's more complicated and you want to handle this test only in a different bug, feel free to.
Josh, I'd like to get this done as makes parts of bug 1311312 simpler. Would you be happy to update the patch, or can I take it from you?

Looks like there's now only four instances of tail files left:

https://dxr.mozilla.org/mozilla-central/search?q=file%3Atail_*.js&redirect=false
Flags: needinfo?(josh)
Please do!
Flags: needinfo?(josh)
Assignee: josh → standard8
Whiteboard: [patchlove] [good first bug][lang=js] → [patchlove]
Comment on attachment 8704833 [details] [diff] [review]
use "do_register_cleanup(...)" calls instead of "tail_*.js" files in xpcshell, in Core and Toolkit; f=sgautherie

Ok, patch updated. Hopefully this will make it this time :-)
Attachment #8704833 - Attachment is obsolete: true
Blocks: 1311312
Comment on attachment 8826208 [details]
Bug 527444 - use do_register_cleanup calls instead of tail_*.js files in xpcshell, in Core and Toolkit. .

https://reviewboard.mozilla.org/r/104198/#review105648

Looks good to me. Nit: I've seen at least one xpcshell.ini without the "tail =" line, I believe it might be optional nowadays.
Attachment #8826208 - Flags: review?(paolo.mozmail) → review+
Comment on attachment 8826208 [details]
Bug 527444 - use do_register_cleanup calls instead of tail_*.js files in xpcshell, in Core and Toolkit. .

https://reviewboard.mozilla.org/r/104198/#review105648

I'm going to remove the tail functionality in bug 503613, so I'll clean those up there.
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/446a150332c3
use do_register_cleanup calls instead of tail_*.js files in xpcshell, in Core and Toolkit. r=Paolo.
https://hg.mozilla.org/mozilla-central/rev/446a150332c3
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.