Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bump commander from v2 to v12 #137

Merged
merged 3 commits into from
Aug 30, 2024
Merged

Bump commander from v2 to v12 #137

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
80201b9
Bump commander from v2 to v12
fregante Aug 4, 2024
a565abf
Skip outdated tests
fregante Aug 4, 2024
4ce588e
Test CLI parsing without user noise
fregante Aug 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 22 additions & 17 deletions bin/fx-runner
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,34 +1,39 @@
#!/usr/bin/env node

var VERSION = require("../package.json").version;
var program = require("commander");
var { program } = require("commander");
var run = require("../lib/run");

program
.version(VERSION)

program
.command("start")
.description("Start Firefox")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.option() must now follow .command()

.option("-b, --binary <path>", "Path of Firefox binary to use.")
.option("--binary-args <CMDARGS>", "Pass additional arguments into Firefox.")
.option("-p, --profile <path>", "Path or name of Firefox profile to use.")
.option("-v, --verbose", "More verbose logging to stdout.")
.option("--new-instance", "Use a new instance")
.option("--no-remote", "Do not allow remote calls")
.option("--foreground", "Bring Firefox to the foreground")
.option("-l, --listen <port>", "Start the debugger server on a specific port.")
.option("-l, --listen <port>", "Start the debugger server on a specific port.", parseInt)
.action(function(options) {
const parsedOptions = {
binary: options.binary || process.env.JPM_FIREFOX_BINARY || "firefox",
profile: options.profile,
"new-instance": !!options.newInstance ? true : false,
"foreground": !!options.foreground ? true : false,
"no-remote": !options.remote ? true : false,
"binary-args": options.binaryArgs || "",
"listen": options.listen || 6000
};

program
.command("start")
.description("Start Firefox")
.action(function() {
console.log(Object.keys(program))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test harness relied on program being outputted to the user. Commander no longer applies the flags to program and they no longer appear as --flags anymore, so the test had to be updated.

I made two changes:

  • the user no longer sees this noise, it's hidden behind a fx-runner-specific ENV
  • instead of looking for random flags, it matches against the whole parsedOptions object. This ensures that the additional logic in this object can also be tested.

run({
binary: program.binary || process.env.JPM_FIREFOX_BINARY || "firefox",
profile: program.profile,
"new-instance": !!program.newInstance ? true : false,
"foreground": !!program.foreground ? true : false,
"no-remote": !program.remote ? true : false,
"binary-args": program.binaryArgs || "",
"listen": program.listen || 6000
})
if (process.env.TESTING_FX_RUNNER) {
console.log(JSON.stringify(parsedOptions));
}

run(parsedOptions)
.then(function(results) {
var firefox = results.process;
if (program.verbose) {
Expand All @@ -37,4 +42,4 @@ program
}, console.exception);
});

program.parse(process.argv);
program.parse();
14 changes: 3 additions & 11 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
"author": "Mozilla Add-ons Team",
"license": "MPL-2.0",
"dependencies": {
"commander": "2.9.0",
"commander": "^12.1.0",
"shell-quote": "1.8.1",
"spawn-sync": "1.0.15",
"when": "3.7.8",
Expand Down
120 changes: 74 additions & 46 deletions test/run/test.run.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,99 +18,127 @@ var parse = require("shell-quote").parse;
var fakeBinary = path.join(__dirname, "..", "utils", "dummybinary" +
(isWindows ? ".bat" : ".sh"));

const ENV = {
env: {
TESTING_FX_RUNNER: true
}
};

describe("fx-runner start", function () {
describe("-b/--binary <FAKE_BINARY>", function () {
it("-p <name>", function (done) {
var proc = exec("start -v -b " + fakeBinary + " -p foo", {}, function (err, stdout, stderr) {
var proc = exec("start -v -b " + fakeBinary + " -p foo", ENV, function (err, stdout, stderr) {
expect(err).to.not.be.ok;
expect(stderr).to.not.be.ok;
expect(stdout).to.contain("-P foo");
expect(stdout).to.not.contain("--P");
expect(stdout).to.not.contain("-foreground");
expect(stdout).to.not.contain("-no-remote");
expect(stdout).to.not.contain("-new-instance");
expect(JSON.parse(stdout)).to.contain({
binary: fakeBinary,
profile: "foo",
"new-instance": false,
"foreground": false,
"no-remote": false,
"binary-args": "",
"listen": 6000
});
done();
});
});

it("-p <path>", function (done) {
var proc = exec("start -v -b " + fakeBinary + " -p ./", {}, function (err, stdout, stderr) {
var proc = exec("start -v -b " + fakeBinary + " -p ./", ENV, function (err, stdout, stderr) {
expect(err).to.not.be.ok;
expect(stderr).to.not.be.ok;
expect(stdout).to.contain("-profile ./");
expect(stdout).to.not.contain("--profile");
expect(stdout).to.not.contain("--P");
expect(stdout).to.not.contain("-foreground");
expect(stdout).to.not.contain("-no-remote");
expect(stdout).to.not.contain("-new-instance");

expect(JSON.parse(stdout)).to.contain({
binary: fakeBinary,
profile: "./",
"new-instance": false,
"foreground": false,
"no-remote": false,
"binary-args": "",
"listen": 6000
});

done();
});
});

it("--binary-args <CMDARGS>", function (done) {
var proc = exec("start -v -b " + fakeBinary + " --binary-args \"-test\" ./", {}, function (err, stdout, stderr) {
var proc = exec("start -v -b " + fakeBinary + " --binary-args \"-test\" ./", ENV, function (err, stdout, stderr) {
expect(err).to.not.be.ok;
expect(stderr).to.not.be.ok;
expect(stdout).to.contain("-test");
expect(stdout).to.not.contain("--binary-args");
expect(stdout).to.not.contain("--profile");
expect(stdout).to.not.contain("--P");
expect(stdout).to.not.contain("-foreground");
expect(stdout).to.not.contain("-no-remote");
expect(stdout).to.not.contain("-new-instance");

expect(JSON.parse(stdout)).to.contain({
binary: fakeBinary,
"new-instance": false,
"foreground": false,
"no-remote": false,
"binary-args": "-test",
"listen": 6000
});
done();
});
});

it("--foreground", function (done) {
var proc = exec("start -v -b " + fakeBinary + " --foreground", {}, function (err, stdout, stderr) {
var proc = exec("start -v -b " + fakeBinary + " --foreground", ENV, function (err, stdout, stderr) {
expect(err).to.not.be.ok;
expect(stderr).to.not.be.ok;
expect(stdout).to.contain("-foreground");
expect(stdout).to.not.contain("--foreground");
expect(stdout).to.not.contain("-P");
expect(stdout).to.not.contain("-no-remote");
expect(stdout).to.not.contain("-new-instance");
expect(JSON.parse(stdout)).to.contain({
binary: fakeBinary,
"new-instance": false,
"foreground": true,
"no-remote": false,
"binary-args": "",
"listen": 6000
});
done();
});
});

it("--no-remote", function (done) {
var proc = exec("start -v -b " + fakeBinary + " --no-remote", {}, function (err, stdout, stderr) {
var proc = exec("start -v -b " + fakeBinary + " --no-remote", ENV, function (err, stdout, stderr) {
expect(err).to.not.be.ok;
expect(stderr).to.not.be.ok;
expect(stdout).to.contain("-no-remote");
expect(stdout).to.not.contain("--no-remote");
expect(stdout).to.not.contain("-P");
expect(stdout).to.not.contain("-foreground");
expect(stdout).to.not.contain("-new-instance");
expect(JSON.parse(stdout)).to.contain({
binary: fakeBinary,
"new-instance": false,
"foreground": false,
"no-remote": true,
"binary-args": "",
"listen": 6000
});
done();
});
});

it("--new-instance", function (done) {
var proc = exec("start -v -b " + fakeBinary + " --new-instance", {}, function (err, stdout, stderr) {
var proc = exec("start -v -b " + fakeBinary + " --new-instance", ENV, function (err, stdout, stderr) {
expect(err).to.not.be.ok;
expect(stderr).to.not.be.ok;
expect(stdout).to.contain("-new-instance");
expect(stdout).to.not.contain("--new-instance");
expect(stdout).to.not.contain("-P");
expect(stdout).to.not.contain("-foreground");
expect(stdout).to.not.contain("-no-remote");
expect(JSON.parse(stdout)).to.contain({
binary: fakeBinary,
"new-instance": true,
"foreground": false,
"no-remote": false,
"binary-args": "",
"listen": 6000
});
done();
});
});

it("--listen", function (done) {
var proc = exec("start -v -b " + fakeBinary + " --listen 6666", {}, function (err, stdout, stderr) {
var proc = exec("start -v -b " + fakeBinary + " --listen 6666", ENV, function (err, stdout, stderr) {
expect(err).to.not.be.ok;
expect(stderr).to.not.be.ok;
expect(stdout).to.contain("-start-debugger-server");
expect(stdout).to.contain("6666");
expect(stdout).to.not.contain("--listen");
expect(stdout).to.not.contain("-P");
expect(stdout).to.not.contain("-foreground");
expect(stdout).to.not.contain("-no-remote");
expect(JSON.parse(stdout)).to.contain({
binary: fakeBinary,
"new-instance": false,
"foreground": false,
"no-remote": false,
"binary-args": "",
"listen": 6666
});
done();
});
});
Expand Down