Closed Bug 1150818 Opened 5 years ago Closed 4 years ago

Need to load mozinfo.json into xpcshell test

Categories

(Testing :: XPCShell Harness, defect)

defect
Not set

Tracking

(firefox41 fixed, firefox42 fixed, firefox43 fixed)

RESOLVED FIXED
mozilla43
Tracking Status
firefox41 --- fixed
firefox42 --- fixed
firefox43 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

(Blocks 2 open bugs)

Details

Attachments

(3 files, 6 obsolete files)

Attached patch mozinfo_in_xpcshell_test.patch (obsolete) β€” β€” Splinter Review
So we can easily skip test on certain conditions.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dfebe25bb031
Attachment #8587858 - Flags: review?(ted)
Blocks: 937740
Comment on attachment 8587858 [details] [diff] [review]
mozinfo_in_xpcshell_test.patch

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

::: testing/xpcshell/runxpcshelltests.py
@@ +421,5 @@
>              '-a', self.appPath,
>              '-r', self.httpdManifest,
>              '-m',
>              '-s',
> +            '-e', 'const MOZINFO = Object.freeze(%s);' % json.dumps(self.mozInfo),

It might be nicer to write out a temp file containing this on startup and pass it with -f instead of serializing this on the command line for every xpcshell invocation.

::: testing/xpcshell/selftest.py
@@ +289,5 @@
>  
> +# A test for mozinfo.
> +LOAD_MOZINFO = '''
> +function run_test() {
> +    do_check_neq(MOZINFO, null);

This doesn't seem like it'd actually fail if MOZINFO were completely missing. Maybe do_check_true(typeof MOZINFO != "undefined") ?

@@ +290,5 @@
> +# A test for mozinfo.
> +LOAD_MOZINFO = '''
> +function run_test() {
> +    do_check_neq(MOZINFO, null);
> +    do_check_neq(MOZINFO.os, null);

Same here.

@@ +903,5 @@
> +    def testLoadMozInfo(self):
> +        """
> +        Check that mozinfo.json is loaded
> +        """
> +        self.writeFile("test_loadMozInfo.js", LOAD_MOZINFO)

I'd just include the JS test body inline here, it's not needed by other tests.
Attachment #8587858 - Flags: review?(ted) → review+
Attached patch Output mozinfo into a js file (obsolete) β€” β€” Splinter Review
This patch might be worth to attach to bug 852207.

Anyway, this patch just adds a method to mozinfo.py to output mozinfo in the following format:

const mozinfo = Object.freeze({"foo": "bar", ...});

So the mozinfo variable can be used in any test frameworks with same manner if the framework uses this method.
Assignee: nobody → hiikezoe
Attachment #8587858 - Attachment is obsolete: true
Attachment #8589326 - Flags: feedback?(ted)
Attached patch Load mozinfo from a temporary file (obsolete) β€” β€” Splinter Review
This patch outputs mozinfo into a temporary js file and loads it as Ted suggested.

The temporary file path is:

On b2g and android:
 /data/local/xpcshell/tmp/mozinfo.js

On others:
 tests-root-dir
 or
 testharnessdir

I don't think the testharnessdir is the best place the tests-root-dir is not defined on try server so I had no choice.

Rest of part of the patch is addressed comment #1.

Thanks Ted!
Attachment #8589333 - Flags: feedback?(ted)
Try server result with above two patches:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b305032eb531
(In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
> On others:
>  tests-root-dir
>  or
>  testharnessdir
> 
> I don't think the testharnessdir is the best place the tests-root-dir is not
> defined on try server so I had no choice.

I meant 'is the best place *but* the tests-root-dir'.
Attachment #8589326 - Flags: feedback?(ted) → feedback+
Comment on attachment 8589333 [details] [diff] [review]
Load mozinfo from a temporary file

Clearing feedback flag.

After some tries to load mozinfo into mochitest framework, I realized including 'const mozinfo = ...' in the mozinfo file is not good idea. In mochitest with e10s, the mozinfo file has to be loaded in chrome process and has to be redefined as a new variable in content process. So the 'const mozinfo' is useless there.

I will attach a new patch against mozbase which just outputs mozinfo *as is* to bug 852207. And I will attach a patch against xpcshell to this bug.

I am sorry for the confusion.
Attachment #8589333 - Attachment is obsolete: true
Attachment #8589333 - Flags: feedback?(ted)
Depends on: 852207
Attached patch Load mozinfo into xpcshell test (obsolete) β€” β€” Splinter Review
This patch defines mozinfo as a global variable by using defineProperty something like lazy getter, there is no overhead if test does not use mozinfo at all.

I am going to request a review after bug 852207 is closed.

Try server result seems good for now:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=79db7a9a5b4b
(In reply to Hiroyuki Ikezoe (:hiro) from comment #7)
> This patch defines mozinfo as a global variable by using defineProperty
> something like lazy getter, there is no overhead if test does not use
> mozinfo at all.

I'd really expect this overhead to be negligible, and moreover the file has already been read to process the manifest and is cached by the OS. I think you can simplify the code by doing the I/O asynchronously in the main xpcshell task, before tests are run:

mozinfo = JSON.parse(yield OS.File.read(_MOZINFO_JS_PATH, { encoding: "utf-8" }));
We're probably splitting hairs here, honestly, it's not likely to make much difference either way.
Blocks: 1167627
Attached patch Output mozinfo into a JSON file (obsolete) β€” β€” Splinter Review
Came back from bug 852207.

This patch is almost the same as attachment 858936. The only difference is the file format. This patch outputs mozinfo as JSON.
Attachment #8589326 - Attachment is obsolete: true
Attachment #8609957 - Flags: review?(ted)
Attached patch Load mozinfo into xpcshell test v2 (obsolete) β€” β€” Splinter Review
Just unrotted.

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

(In reply to :Paolo Amadini from comment #8)

> mozinfo = JSON.parse(yield OS.File.read(_MOZINFO_JS_PATH, { encoding:
> "utf-8" }));

I tried it but it needs other works to use osfile.jsm in testing/xpshell/head.js. Please file a new bug if you need it. I will care it later.
Attachment #8591491 - Attachment is obsolete: true
Attachment #8609957 - Attachment is obsolete: true
Attachment #8609957 - Flags: review?(ted)
Attachment #8609958 - Flags: review?(ted)
Comment on attachment 8609957 [details] [diff] [review]
Output mozinfo into a JSON file

Sorry. I accidentally marked this patch as obsolete.
Attachment #8609957 - Attachment is obsolete: false
Attachment #8609957 - Flags: review?(ted)
No longer depends on: 852207
Attachment #8609957 - Flags: review?(ted) → review+
Comment on attachment 8609958 [details] [diff] [review]
Load mozinfo into xpcshell test v2

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

::: testing/xpcshell/head.js
@@ +1510,5 @@
> +  let json = Components.classes["@mozilla.org/dom/json;1"]
> +    .createInstance(Components.interfaces.nsIJSON);
> +  let mozinfo = json.decodeFromStream(stream, stream.available());
> +  stream.close();
> +  return mozinfo;

Wow, I had forgotten how terrible XPCOM File I/O is. I wish we could use OS.File here, but that'd require making this async, which seems like more of a hassle.

::: testing/xpcshell/runxpcshelltests.py
@@ +603,5 @@
>          # Create a profile and a temp dir that the JS harness can stick
>          # a profile and temporary data in
>          self.profileDir = self.setupProfileDir()
>          self.tempDir = self.setupTempDir()
> +        self.setupMozinfoJS()

As per the lines above, it would be nice if setupMozinfoJS returned the path, and you assigned it here.
Attachment #8609958 - Flags: review?(ted) → review+
Rebased to master.
Attachment #8609957 - Attachment is obsolete: true
Attachment #8650742 - Flags: review+
Blocks: 1153126
Rebased to master. And setupMozInfoJS returns the path.
Attachment #8609958 - Attachment is obsolete: true
Attachment #8650756 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a5c3c5566eec
https://hg.mozilla.org/mozilla-central/rev/b0d464df0f0b
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
seems this cause conflicts when moving to beta :

grafting 258858:a5c3c5566eec "Bug 1150818 - Part 1: Output mozinfo into a file. r=ted"
merging testing/mozbase/mozinfo/mozinfo/mozinfo.py
3 files to edit
merging testing/mozbase/mozinfo/mozinfo/mozinfo.py failed!
merging testing/mozbase/mozinfo/tests/test.py                                                                                                   


Fallen could you take a look,thanks!
Flags: needinfo?(philipp)
Here is part 1 applied to beta. It conflicted because of the not yet existing StringVariables code.
Flags: needinfo?(philipp)
Attachment #8657809 - Flags: review+
Attachment #8657809 - Attachment is patch: true
You need to log in before you can comment on or make changes to this bug.