Move TestAUSReadStrings from 'make check' to moz.build / xpcshell

RESOLVED FIXED in Firefox 54

Status

()

Core
Build Config
RESOLVED FIXED
8 months ago
8 months ago

People

(Reporter: mshal, Assigned: mshal)

Tracking

(Blocks: 1 bug)

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

8 months ago
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.
(Assignee)

Updated

8 months ago
Depends on: 1340720
(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

8 months 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

8 months ago
Assignee: nobody → mshal
(Assignee)

Updated

8 months ago
No longer depends on: 1340720
Comment hidden (mozreview-request)

Comment 4

8 months 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

8 months 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 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)

Comment 8

8 months ago
Pushed by mshal@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b905df59d733
Move TestAUSReadStrings into ausReadStrings.js; r=rstrong,ted

Comment 9

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b905df59d733
Status: NEW → RESOLVED
Last Resolved: 8 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
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)
Sorry, please ignore my NI, I just found the binary in common.tests.zip.
Flags: needinfo?(mshal)
You need to log in before you can comment on or make changes to this bug.