fix: print a development version when using a custom web-ext build #565

saintsebastian · 2016-10-10T20:10:21Z

Fixes #481

coveralls · 2016-10-10T20:19:10Z

Coverage decreased (-0.3%) to 99.688% when pulling 17957f1 on saintsebastian:fix-481 into 8e4e657 on mozilla:master.

…th isolated tests

kumar303

I requested a few changes but there is a bigger problem (as you mentioned on IRC) about NODE_ENV. This variable is defined during npm run build but the web-ext version is not generated during npm run build. Oops, this was an oversight. We need to think of a different approach.

kumar303 · 2016-10-10T20:24:25Z

tests/unit/test.program.js

-        assert.equal(defaultVersionGetter(root),
-                     JSON.parse(pkgData).version);
-      });
+    .then((pkgData) => {


Just a minor problem here: this block should be indented two spaces to show that .then() is a continuation of the call to readFile().

kumar303 · 2016-10-10T20:27:34Z

tests/unit/test.program.js

+
+  it('returns git commit information in development', () => {
+    const commit = `${git.branch()}-${git.long()}`;
+    assert.equal(defaultVersionGetter(root),


This should be called using a testProcess object just like before. Whenever you have a test like this that relies on a specific state of a dependency (i.e. process) you need to explicitly set up that state, even though it's supposed to be the default state. Something like this:

const testProcess = { // Empty env to simulate not declaring NODE_ENV. env: {}, }; defaultVersionGetter(root, testProcess)

kumar303 · 2016-10-10T20:35:51Z

src/program.js

+  localProcess: Object = process
+): string {
+  if (localProcess.env.NODE_ENV === 'production') {
+    log.debug('Getting the version from package.json');


oops, the code needs this fix to make the log statements show up in --verbose mode:

diff --git a/src/program.js b/src/program.js index eb9e8ae..5bf90ae 100644 --- a/src/program.js +++ b/src/program.js @@ -92,8 +92,8 @@ export class Program { let runCommand = this.commands[cmd]; if (argv.verbose) { - log.info('Version:', getVersion(absolutePackageDir)); logStream.makeVerbose(); + log.info('Version:', getVersion(absolutePackageDir)); } try {

coveralls · 2016-10-10T21:01:11Z

Coverage remained the same at 100.0% when pulling ee764a6 on saintsebastian:fix-481 into 8e4e657 on mozilla:master.

kumar303 · 2016-10-10T21:09:40Z

One thing I can think of is to define a magic global at compile time (when npm run build executes), using the DefinePlugin. You could add this to webpack.config.js:

new webpack.DefinePlugin({
  WEBEXT_BUILD_ENV: JSON.stringify(process.env.NODE_ENV || 'development'),
})

kumar303 · 2016-10-10T21:13:16Z

tests/unit/test.program.js

-  it('returns the package version', () => {
-    let root = path.join(__dirname, '..', '..');
+  let root = path.join(__dirname, '..', '..');
+  let testProcess = {


This shouldn't be shared be a shared variable because it makes the tests unpredictable (say, if they were run in reverse order). I'd say keep it defined as a local constant within each respective test function.

…version

coveralls · 2016-10-11T10:18:54Z

Coverage remained the same at 100.0% when pulling c193075 on saintsebastian:fix-481 into 8e4e657 on mozilla:master.

kumar303

Nice work on the tests! I just requested some minor changes and one issue from the last review still needs to be resolved.

kumar303 · 2016-10-11T22:41:33Z

src/program.js

@@ -123,11 +125,21 @@ export class Program {
  }
 }

+declare var WEBEXT_BUILD_ENV: string;


It might be worth adding a comment above this explaining how this is generated by webpack and that you can see how it's generated in webpack.config.js.

kumar303 · 2016-10-11T22:44:39Z

tests/unit/test.program.js

@@ -166,6 +168,7 @@ describe('program.Program', () => {

  it('configures the logger when verbose', () => {
    const logStream = fake(new ConsoleStream());
+    let version = spy();


always use const if the variable does not need to be modified

kumar303 · 2016-10-11T22:48:30Z

src/program.js

-  return JSON.parse(packageData).version;
+export function defaultVersionGetter(
+  absolutePackageDir: string,
+  localEnv: string = WEBEXT_BUILD_ENV


Since this is an optional argument, it should be defined as a keyword argument instead. This help makes the calls easier to read and it makes it easy to add more options later on. Example:

export function defaultVersionGetter( absolutePackageDir: string, {localEnv: string = WEBEXT_BUILD_ENV} = {} ): string {

I committed in this form, but it breaks the eslint tests. Should i insert inline rule disable comments?

kumar303 · 2016-10-11T22:51:10Z

tests/unit/test.program.js

@@ -175,7 +178,7 @@ describe('program.Program', () => {
    });
    program.command('thing', 'does a thing', () => {});

-    return execProgram(program, {logStream})
+    return execProgram(program, {getVersion: version, logStream})


it looks like this can just be getVersion: spy() since the version variable isn't used elsewhere

kumar303 · 2016-10-14T16:04:34Z

src/program.js

-  return JSON.parse(packageData).version;
+export function defaultVersionGetter(
+  absolutePackageDir: string,
+  {localEnv: string = WEBEXT_BUILD_ENV} = {}


Oh, sorry, I gave you the wrong example code. The lint error is correct and it needs to be fixed. The problem is actually an obscure ES6 vs Flow problem so let me offer the solution to help you move forward.

First, define a type for the option object that needs to be passed to defaultVersionGetter(), like this:

type versionGetterOptions = { localEnv?: string, };

Next, fix the definition of defaultVersionGetter to reference the new type:

export function defaultVersionGetter( absolutePackageDir: string, {localEnv = WEBEXT_BUILD_ENV}: versionGetterOptions = {} ): string {

After this change, you'll need to update all the calls to defaultVersionGetter(). The flow checks in npm start should help guide you on what to change and where.

Updated just the test, default argument took care of all other calls. Seems to be finally working.

coveralls · 2016-10-14T16:45:56Z

Coverage remained the same at 100.0% when pulling bdd0929 on saintsebastian:fix-481 into 8e4e657 on mozilla:master.

kumar303

Thanks for the quick follow-ups, this looks great. I added some very minor nit-picky comments just for future reference. You don't need to fix these up.

kumar303 · 2016-10-14T17:28:14Z

src/program.js

-  let packageData: any = readFileSync(
-    path.join(absolutePackageDir, 'package.json'));
-  return JSON.parse(packageData).version;
+//A defintion of type of argument for defaultVersionGetter


Just for future note: comments typically begin with a space elsewhere in the codebase, like // A definition...

kumar303 · 2016-10-14T17:29:06Z

tests/unit/test.program.js

-        assert.equal(defaultVersionGetter(root),
-                     JSON.parse(pkgData).version);
+        const testBuildEnv = {localEnv: 'production'};
+        assert.equal(defaultVersionGetter(root, testBuildEnv),


You could make this a bit more concise like:

assert.equal(defaultVersionGetter(root, {localEnv: 'production'})

Not a big deal though, don't worry about changing it.

I was on the fence about doing that, thanks for clarification.

Also I don't mind fixing these, I've committed them to my branch in case you have time to merge them as well.

hmm, it's a bit tricky to pick those from a different branch. No worries! They aren't important changes but thanks.

atsay · 2016-10-20T22:03:59Z

I've added your contribution to our recognition wiki--thanks, and welcome to our project!

fix: print a development version when using a custom web-ext build

17957f1

muffinresearch added the state: pull request ready label Oct 10, 2016

saintsebastian closed this Oct 10, 2016

muffinresearch removed the state: pull request ready label Oct 10, 2016

saintsebastian added 2 commits October 10, 2016 22:33

fix: print a development version when using a custom web-ext build

a0c20b3

fix: print a development version when using a custom web-ext build wi…

ee764a6

…th isolated tests

kumar303 reviewed Oct 10, 2016

View reviewed changes

kumar303 mentioned this pull request Oct 10, 2016

fix: print a development version when using a custom web-ext build #568

Closed

kumar303 reopened this Oct 10, 2016

kumar303 added the state: pull request ready label Oct 10, 2016

kumar303 reviewed Oct 10, 2016

View reviewed changes

fix: global environment variable for printing production/development …

c193075

…version

kumar303 suggested changes Oct 11, 2016

View reviewed changes

fix: minor fixes for correct version printing

d30d52f

kumar303 reviewed Oct 14, 2016

View reviewed changes

fix: new flow type for version printing

bdd0929

kumar303 approved these changes Oct 14, 2016

View reviewed changes

kumar303 merged commit 1fcb2e2 into mozilla:master Oct 14, 2016

kumar303 removed the state: pull request ready label Oct 14, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: print a development version when using a custom web-ext build #565

fix: print a development version when using a custom web-ext build #565

saintsebastian commented Oct 10, 2016

coveralls commented Oct 10, 2016 •

edited

Loading

kumar303 left a comment

kumar303 Oct 10, 2016

kumar303 Oct 10, 2016

kumar303 Oct 10, 2016

coveralls commented Oct 10, 2016 •

edited

Loading

kumar303 commented Oct 10, 2016 •

edited

Loading

kumar303 Oct 10, 2016

coveralls commented Oct 11, 2016 •

edited

Loading

kumar303 left a comment

kumar303 Oct 11, 2016

kumar303 Oct 11, 2016

kumar303 Oct 11, 2016 •

edited

Loading

saintsebastian Oct 12, 2016

kumar303 Oct 11, 2016

kumar303 Oct 14, 2016

saintsebastian Oct 14, 2016

coveralls commented Oct 14, 2016 •

edited

Loading

kumar303 left a comment

kumar303 Oct 14, 2016

kumar303 Oct 14, 2016

saintsebastian Oct 14, 2016

saintsebastian Oct 14, 2016

kumar303 Oct 14, 2016

atsay commented Oct 20, 2016

fix: print a development version when using a custom web-ext build #565

fix: print a development version when using a custom web-ext build #565

Conversation

saintsebastian commented Oct 10, 2016

coveralls commented Oct 10, 2016 • edited Loading

kumar303 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 10, 2016 • edited Loading

kumar303 commented Oct 10, 2016 • edited Loading

Choose a reason for hiding this comment

coveralls commented Oct 11, 2016 • edited Loading

kumar303 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumar303 Oct 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 14, 2016 • edited Loading

kumar303 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atsay commented Oct 20, 2016

coveralls commented Oct 10, 2016 •

edited

Loading

coveralls commented Oct 10, 2016 •

edited

Loading

kumar303 commented Oct 10, 2016 •

edited

Loading

coveralls commented Oct 11, 2016 •

edited

Loading

kumar303 Oct 11, 2016 •

edited

Loading

coveralls commented Oct 14, 2016 •

edited

Loading