Closed Bug 1340699 Opened 3 years ago Closed 3 years ago

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

Categories

(Firefox Build System :: General, defect)

defect
Not set

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.
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.
(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: nobody → mshal
No longer depends on: 1340720
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+
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+
Pushed by mshal@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b905df59d733
Move TestAUSReadStrings into ausReadStrings.js; r=rstrong,ted
https://hg.mozilla.org/mozilla-central/rev/b905df59d733
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Sorry, please ignore my NI, I just found the binary in common.tests.zip.
Flags: needinfo?(mshal)
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.