browser chrome MochiTest harness doesn't permit |let foo = is; foo()|

RESOLVED FIXED in mozilla1.9

Status

P3
normal
RESOLVED FIXED
11 years ago
a year ago

People

(Reporter: myk, Assigned: Gavin)

Tracking

Trunk
mozilla1.9

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

11 years ago
Created attachment 298375 [details] [diff] [review]
patch v1: adds todo_is, todo_isnot functions

In non-browser chrome MochiTests, you can assign a test function to a variable and then invoke the function through the variable:

let foo = is;
foo(true, "you can call test function through variable");

But in browser chrome MochiTests, doing that throws "TypeError: this.ok is not a function", presumably because "this" is not set to the scope object when the function is called through the variable and the "is" function calls "this.ok".

I thought perhaps removing the reference to "this" from the ok function would fix the problem, since "ok" is in the scope of the test code, but it apparently isn't in the scope of the scope object's code, since that throws "ReferenceError: ok is not defined".

It's unclear whether that represents a bug in the subscript loader code, but in any case it would be useful for browser chrome tests to have this capability, since it would enable them to dynamically choose between a test function and its todo_* equivalent based on a value in a data structure or returned from a function, as non-browser chrome MochiTests frequently do, f.e. in layout/style/test/test_value_storage.html:

    func = xfail_accepted(property, value) ? todo_isnot : isnot;
    func(step1val, "", "setting '" + value + "' on '" + property);

(Perhaps this works only because SimpleTest.js uses no "this" references).

In the meantime, the ugly workaround is to do:

let foo = "is";
this[foo](true, "you can call test function through variable");
Attachment #298375 - Flags: review?(gavin.sharp)
(Reporter)

Comment 1

11 years ago
Comment on attachment 298375 [details] [diff] [review]
patch v1: adds todo_is, todo_isnot functions

Ignore the attachment.  It belongs to another bug.  I used the same form to file both, reloading it before filing this one, and the "Enter Bug" form apeared to revert to the "no attachment" state after the reload, but apparently appearances were deceiving.  I have filed Bugzilla bug 413419 on the problem.
Attachment #298375 - Attachment is obsolete: true
Attachment #298375 - Flags: review?(gavin.sharp)
Created attachment 298486 [details] [diff] [review]
patch

Set the test functions on the instance rather than on the prototype, using a closure. This conflicts with your patch in bug 413416, but merging is trivial.
Assignee: myk → gavin.sharp
Status: NEW → ASSIGNED
Attachment #298486 - Flags: review?(myk)
Attachment #298486 - Flags: review?(jwalden+bmo)
Oops, hadn't noticed that you'd assigned this to yourself - was that intentional, or another artifact of the form-reloading? Didn't mean to step on any toes, I'm certainly open to alternative solutions.
Version: unspecified → Trunk

Updated

11 years ago
Attachment #298486 - Flags: review?(jwalden+bmo) → review+
(Reporter)

Comment 4

11 years ago
Comment on attachment 298486 [details] [diff] [review]
patch

(In reply to comment #3)
> Oops, hadn't noticed that you'd assigned this to yourself - was that
> intentional, or another artifact of the form-reloading?

It was just an artifact, and I'm happy for you to take the bug.


(In reply to comment #2)

> Set the test functions on the instance rather than on the prototype, using a
> closure. This conflicts with your patch in bug 413416, but merging is trivial.

This looks like a fine fix, and no worries about the conflict in the other bug.  I can deal with the merging.
Attachment #298486 - Flags: review?(myk) → review+
Priority: -- → P3
Target Milestone: --- → mozilla1.9 M11
Comment on attachment 298486 [details] [diff] [review]
patch

Low risk test harness change (on the off change that something does go wrong, it will go catastrophically wrong and this can be backed out).
Attachment #298486 - Flags: approval1.9?

Updated

11 years ago
Attachment #298486 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
(Reporter)

Comment 6

11 years ago
Checked in:

Checking in testing/mochitest/browser-test.js;
/cvsroot/mozilla/testing/mochitest/browser-test.js,v  <--  browser-test.js
new revision: 1.4; previous revision: 1.3
done
Checking in testing/mochitest/tests/browser/browser_pass.js;
/cvsroot/mozilla/testing/mochitest/tests/browser/browser_pass.js,v  <--  browser_pass.js
new revision: 1.2; previous revision: 1.1
done
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Component: Testing → BrowserTest
Product: Core → Testing
QA Contact: testing → browsertest
Target Milestone: mozilla1.9beta3 → mozilla1.9
Component: BrowserTest → Mochitest
Product: Testing → Testing
You need to log in before you can comment on or make changes to this bug.