error in test.waitUntil callback passes silently

RESOLVED FIXED in 1.3

Status

Add-on SDK
General
P2
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: markh, Assigned: markh)

Tracking

unspecified

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

Created attachment 556444 [details] [diff] [review]
Better handling of failures in assertion function

To reproduce, add the following test to the end of packages/api-utils/tests/test-unit-test.js:

+exports.testWaitUntilErrorInCallback = function(test) {
+  test.waitUntilDone();
+
+  test.waitUntil(function () {throw "oops"}, "waitUntil pass")
+      .then(function () test.done());
+}

As you can see the callback function throws an error - however, if you run the test suite with this modification it is reported that all tests pass.  The desired behaviour is that a traceback to the error is generated and the test fails.

I'm attaching a patch which fixes this - after this patch the new test correctly fails..  However, note that the attached patch does *not* have tests - it isn't clear to me how to write a test where the expected behaviour is the test failing.  IOW the above test isn't suitable as once the patch is applied the test suite then correctly fails.
Shane has a pull request that adds the ability to mark as test as "expected to fail".  It's been reviewed, but it needs a few changes before it can land. cc:ing him for status.
Thanks Myk, could you have https://github.com/shane-tomlinson/addon-sdk/commit/6ba99a4e2d5f68e66dbf1033fe169896301b49b0#packages/api-utils/tests/test-unit-test.js-P4 - it is my note asking if you know of a better way to write a test to check the done function being called in teardown for async tests.  Once that is cleared up, the pull request should be ready for another review.
Assignee: nobody → mhammond
Priority: -- → P2
Target Milestone: --- → 1.2
(Pushing all open bugs to the --- milestone for the new triage system)
Target Milestone: 1.2 → ---
(Assignee)

Comment 4

6 years ago
Created attachment 560382 [details] [diff] [review]
Patch with fix and test leveraging test.expectFail

Same fix as before, but now includes a test which leverages test.expectFail.  Requesting review from Shane to share the review love :)
Attachment #560382 - Flags: review?(stomlinson)
Comment on attachment 560382 [details] [diff] [review]
Patch with fix and test leveraging test.expectFail

Mark,
  This looks good to me, but I am going to pass this to Myk since he is the Jetpack overlord and has a much clearer understanding of how all the parts fit together.
Attachment #560382 - Flags: review?(stomlinson)
Attachment #560382 - Flags: review?(myk)
Attachment #560382 - Flags: review+
Comment on attachment 560382 [details] [diff] [review]
Patch with fix and test leveraging test.expectFail

(In reply to Shane Tomlinson from comment #5)
>   This looks good to me, but I am going to pass this to Myk since he is the
> Jetpack overlord and has a much clearer understanding of how all the parts
> fit together.

Heh, I wish.


>diff --git a/packages/api-utils/tests/test-unit-test.js b/packages/api-utils/tests/test-unit-test.js

>+exports.testWaitUntilErrorInCallback = function(test) {
>+  test.waitUntilDone();
>+
>+  test.expectFail(function() {
>+    test.waitUntil(function () {throw "oops"}, "waitUntil pass")
>+        .then(function () test.done());
>+  })
>+}

Looks good modulo a couple trivial formatting nits (f.e. closing semi-colon on outermost statement). r=myk!
Attachment #560382 - Flags: review?(myk) → review+
https://github.com/mozilla/addon-sdk/commit/9b6e6679c52d6511740da4fb9fa58847c3345291
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [cherry-pick-wanted]
Erm, expectFail actually landed after we branched for stabilization of 1.2, and it's too big a change to take during stabilization, so we can't cherry-pick this to 1.2 after all.  1.3 it is!
Whiteboard: [cherry-pick-wanted]
Target Milestone: --- → 1.3
You need to log in before you can comment on or make changes to this bug.