Closed Bug 1187701 Opened 5 years ago Closed 5 years ago

Implement add_task function for mochitest chrome and plain

Categories

(Testing :: General, defect)

All
Unspecified
defect
Not set

Tracking

(firefox42 fixed)

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: arthur, Assigned: arthur)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [tor])

Attachments

(1 file, 1 obsolete file)

In Bug 1078657 we implemented a spawnTask function, similar to Task.spawn() in Task.jsm, that allows asynchronous tasks to be launched from within mochitest-plain and mochitest-chrome tests. In this bug, we'd like to implement the add_task function, as is available in mochitest-browser. This function is similar to spawnTask, but in addition ensures that SimpleTest.waitForExplicitFinish() is called before the tasks start, and SimpleTest.finish() is called after all tasks are done.
Blocks: 1186277
In this patch, I've renamed spawnTask to spawn_task, to be consistent with add_task.
Attachment #8640826 - Flags: review?(jmaher)
Comment on attachment 8640826 [details] [diff] [review]
0001-Bug-1187701-add_task-function-for-mochitest-chrome-a.patch

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

this assumes that we will only be calling add_task and that the user doesn't put a waitforexplicitfinish() call themselves and a finish() call themselves, right?  How can we be guaranteed that?

I would like more background on this prior to a r+.  Codewise it is nice to see the tests added, are there failure cases we could add here (look for fail-if condition in mochitest.ini).  I would like to see some blank lines in the __add_task function so that we can read it easier.  

This seems fragile with no safeguards if developers start mixing in random add_tasks to their tests.

::: testing/mochitest/tests/SimpleTest/SpawnTask.js
@@ +242,5 @@
>  
>  return co;
>  })();
> +
> +// __add_task(generatorFunction)__

why do we have __ before/after the definition?
Attachment #8640826 - Flags: review?(jmaher) → review-
(In reply to Joel Maher (:jmaher) from comment #3)
> this assumes that we will only be calling add_task and that the user doesn't
> put a waitforexplicitfinish() call themselves and a finish() call
> themselves, right?  How can we be guaranteed that?

An extra call to waitForExplicitFinish() should be a no-op I think. An extra call to finish() should trigger the "called finish() multiple times" warning -- pretty obvious to me.

Are there additional safeguards for the xpcshell or mochitest-browser add_tasks?

I've had nothing but success using this add_task in bug 1184186 and found it intuitive and useful. I'm certainly in favor of improvements, but I'm not sure I see what can be done here.
I was not fully aware of the implementation and usage in xpcshell and mochitest-browser, thanks for bringing that up.  given that, if this is in place and used, there is probably no real issue with adding this to mochitest chrome/plain.

I still would like to know about the __ usage.
(In reply to Joel Maher (:jmaher) from comment #5)
> I was not fully aware of the implementation and usage in xpcshell and
> mochitest-browser, thanks for bringing that up.  given that, if this is in
> place and used, there is probably no real issue with adding this to
> mochitest chrome/plain.
> 
> I still would like to know about the __ usage.

This was just intended as a markdown-style emphasis in the comment text. The actual function definition starts a few lines below. I think you're right that the underscores are confusing, so I've removed them.

try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba81e9c9c8f8
Attachment #8640826 - Attachment is obsolete: true
Attachment #8642442 - Flags: review?(jmaher)
Comment on attachment 8642442 [details] [diff] [review]
0001-Bug-1187701-add_task-function-for-mochitest-chrome-a.patch

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

ok, lets get this moving forward.
Attachment #8642442 - Flags: review?(jmaher) → review+
(In reply to Joel Maher (:jmaher) from comment #7)
> Comment on attachment 8642442 [details] [diff] [review]
> 0001-Bug-1187701-add_task-function-for-mochitest-chrome-a.patch
> 
> Review of attachment 8642442 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ok, lets get this moving forward.

Thanks for the reviews!
Keywords: checkin-needed
Assignee: nobody → arthuredelstein
https://hg.mozilla.org/mozilla-central/rev/4e05c3afe0e0
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
(In reply to Martijn Wargers [:mwargers] (QA) from comment #12)
> How does this add_task thing work?
> I guess similar like here?
> https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-
> based_unit_tests#XPCShell_test_utility_functions

Yes, the add_task function should work the same.

For mochitest (plain) you need to add

   <script type="text/javascript" src="/tests/SimpleTest/SpawnTask.js"></script>

Here's an example: https://dxr.mozilla.org/mozilla-central/source/modules/libjar/test/mochitest/test_bug1173171.html

For mochitest (chrome) use

   <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/SpawnTask.js"></script>

For example: https://dxr.mozilla.org/mozilla-central/source/mobile/android/tests/browser/chrome/test_accounts.html
Blocks: 1256297
Whiteboard: [tor]
Blocks: meta_tor
You need to log in before you can comment on or make changes to this bug.