Closed
Bug 1340699
Opened 7 years ago
Closed 7 years ago
Move TestAUSReadStrings from 'make check' to moz.build / xpcshell
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(firefox54 fixed)
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: mshal, Assigned: mshal)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
I'd like to move this test out of the Makefile since it's one of the few that blocks us from at least turning on 'make check' for cross-OSX builds (in addition to obviously stopping us from getting rid of 'make check' entirely). I think this is possible by adding a js file to run the test in xpcshell, but I hit a snag with the fact that we use a unicode character in the directory on Windows. I'll file a separate bug for that.
Comment 1•7 years ago
|
||
(In reply to Michael Shal [:mshal] from comment #0) > I think this is possible by adding a js file to run the test in xpcshell, > but I hit a snag with the fact that we use a unicode character in the > directory on Windows. I'll file a separate bug for that. If you invoked the test from an xpcshell test you could just have it create a temporary directory with the unicode pathname, copy the test binary there, and run it from there.
Assignee | ||
Comment 2•7 years ago
|
||
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #1) > If you invoked the test from an xpcshell test you could just have it create > a temporary directory with the unicode pathname, copy the test binary there, > and run it from there. Yeah I had the same thought as well, and played around with it yesterday. It seems to be much easier than getting unicode working in python2 :)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → mshal
Comment hidden (mozreview-request) |
Comment 4•7 years ago
|
||
mozreview-review |
Comment on attachment 8841236 [details] Bug 1340699 - Move TestAUSReadStrings into ausReadStrings.js; https://reviewboard.mozilla.org/r/115510/#review117996 Hooray for removing a bunch of Makefile.in gunk while you're at it! It might not hurt to have whoever wrote this test give this a once-over just to sanity check, but this seems right to me.
Attachment #8841236 -
Flags: review?(ted) → review+
Assignee | ||
Comment 5•7 years ago
|
||
Comment on attachment 8841236 [details] Bug 1340699 - Move TestAUSReadStrings into ausReadStrings.js; :rstrong, can you take a look at the new js file to invoke TestAUSReadStrings? I don't normally write js, so I wouldn't be surprised if there are some issues.
Attachment #8841236 -
Flags: review?(robert.strong.bugs)
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8841236 [details] Bug 1340699 - Move TestAUSReadStrings into ausReadStrings.js; https://reviewboard.mozilla.org/r/115510/#review118626 Looks good... mainly just nits. r=me with these changes. ::: toolkit/mozapps/update/tests/unit_aus_update/ausReadStrings.js:11 (Diff revision 1) > +const BIN_EXE = "TestAUSReadStrings" + BIN_SUFFIX; > +const tempdir = do_get_tempdir(); > + > +function run_test() { > + setupTestCommon(); > + standardInit(); No need to call setupTestCommon or standardInit for this test. They are only needed for the tests that use the nsIUpdateService. ::: toolkit/mozapps/update/tests/unit_aus_update/ausReadStrings.js:16 (Diff revision 1) > + standardInit(); > + > + var workdir = tempdir.clone(); > + workdir.append(BIN_DIR); > + > + var paths = [ nit: go ahead and use let instead of var. ::: toolkit/mozapps/update/tests/unit_aus_update/ausReadStrings.js:27 (Diff revision 1) > + for (let i = 0; i < paths.length; i++) { > + let file = do_get_file("../data/" + paths[i]); > + file.copyTo(workdir, null) > + } > + > + var readStrings = workdir.clone(); nit: let instead of var here as well. ::: toolkit/mozapps/update/tests/unit_aus_update/ausReadStrings.js:30 (Diff revision 1) > + } > + > + var readStrings = workdir.clone(); > + readStrings.append(BIN_EXE); > + > + var process = Components.classes["@mozilla.org/process/util;1"] nit: let instead of var here as well. Also, Cc and Ci are available so just write this as follows which is short enough for a single line: let process = Cc["@mozilla.org/process/util;1"].createInstance(Ci.nsIProcess); ::: toolkit/mozapps/update/tests/unit_aus_update/ausReadStrings.js:36 (Diff revision 1) > + .createInstance(Components.interfaces.nsIProcess); > + process.init(readStrings); > + process.run(true, [], 0); > + do_check_eq(process.exitValue, 0); > + > + doTestFinish(); Remove the call to doTestFinish since it isn't needed without the call to setupTestCommon.
Attachment #8841236 -
Flags: review?(robert.strong.bugs) → review+
Comment hidden (mozreview-request) |
Pushed by mshal@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b905df59d733 Move TestAUSReadStrings into ausReadStrings.js; r=rstrong,ted
Comment 9•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b905df59d733
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment 10•7 years ago
|
||
The binary TestAUSReadStrings(.exe) is not included in xpcshell test package, e.g., https://queue.taskcluster.net/v1/task/C3D_f4LZTEml2JWw50y6BA/artifacts/public/build/firefox-55.0a1.en-US.win32.xpcshell.tests.zip, https://queue.taskcluster.net/v1/task/UFQ0a7zRSWSn3SzFEDLw5Q/artifacts/public/build/target.xpcshell.tests.zip which copy/run it actually has no effect...
Flags: needinfo?(mshal)
Comment 11•7 years ago
|
||
Sorry, please ignore my NI, I just found the binary in common.tests.zip.
Flags: needinfo?(mshal)
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•