Support warnings-as-errors #613

josteink · 2016-10-26T19:23:29Z

This patch adds support for using --warnings-as-errors as a parameter for the lint command.

This enables the functionality provided in the PR mozilla/addons-linter#1016 and partially resolves the issue mozilla/addons-linter#1014.

Once addons-linter has PR accepted and merged, packages.json will also need to have the package-version bumped to fully resolve and close the issue.

coveralls · 2016-10-26T19:41:58Z

Coverage remained the same at 100.0% when pulling 16103e9 on josteink:warnings-as-errors into fd662af on mozilla:master.

coveralls · 2016-10-28T17:46:59Z

Coverage remained the same at 100.0% when pulling d9db669 on josteink:warnings-as-errors into fd662af on mozilla:master.

kumar303

Thanks for adding this feature! It just needs a quick test to make sure the command option {warningsAsErrors: true} is hooked up to the linter configuration. You can follow this as an example: https://github.com/mozilla/web-ext/blob/master/tests/unit/test-cmd/test.lint.js#L68-L75

josteink · 2016-10-29T18:45:08Z

I originally just tested that it worked as expected against the patched addon-signer from the command-line, but I can absolutely respect the desire for automated tests.

I'll be 100% honest and admit that I don't fully understand how those tests are composed and how they work in this project, and I don't have a webpack-decoding capable debugger to dig into it with...

But with that said, I've added some code which (to me) does look reasonable and which "tests green". Feel free to let me know what you think about it. Does it look correct? Does it suffice?

if so, we just need to wait for addon-signer to accept my other patch and bump their version and we should be good to go :)

coveralls · 2016-10-29T18:58:55Z

Coverage remained the same at 100.0% when pulling 446aa8f on josteink:warnings-as-errors into fd662af on mozilla:master.

kumar303 · 2016-10-30T10:53:10Z

I'll be 100% honest and admit that I don't fully understand how those tests are composed and how they work in this project

The npm start command as documented will run the entire test suite. Let me know if you run into any trouble with it.

kumar303

Thanks for adding the tests! I requested a few minor changes.

kumar303 · 2016-10-30T12:32:14Z

tests/unit/test-cmd/test.lint.js

@@ -96,13 +104,16 @@ describe('lint', () => {
      boring: 'boring flag',
      // $FLOW_IGNORE: wrong type used for testing purpose
      selfHosted: 'self-hosted flag',
+      // $FLOW_IGNORE: wrong type used for testing purpose
+      warningsAsErrors: 'warnings-as-errors flag',


you don't need to add this parameter to the test because the "pass through" is already covered by the first test you added. Also, this pass through test is kind of silly.

I think you can remove this entirely. Or is there any reason to include it? Do the tests fail?

kumar303 · 2016-10-30T12:33:10Z

tests/unit/test-cmd/test.lint.js

+      const config = createLinter.firstCall.args[0].config;
+      assert.equal(config.warningsAsErrors, true);
+    });
+  });


this looks great! I think it would make sense to add one more test that does lint() without specifying the warnings flag and asserts the linter is called with warningsAsErrors = undefined (or should it be false?)

josteink · 2016-10-30T12:46:52Z

Like i tried to say: I've added some tests and they all run fine. I'm just not 100% confident about how all the bits and pieces which the tests are built from fit together, so I cannot know for certain that what I'd like to test gets tested.

If you could look over my commits and double check that you think it makes sense, that would be greatly appreciated.

josteink · 2016-10-30T13:01:41Z

Ok. Thanks for the feedback.

I agree the pass-through tests looks kind of silly but I decided to add them just for the sake of consistency. I'll add another test to verify undefined behavior.

Thanks again for your help.

kumar303 · 2016-10-30T13:28:06Z

Also, if you merge in the latest master, I fixed the lint error (which is unrelated to your patch).

coveralls · 2016-10-30T19:19:07Z

Coverage remained the same at 100.0% when pulling b7460cc on josteink:warnings-as-errors into 0b9730f on mozilla:master.

josteink · 2016-10-31T12:01:46Z

And there everything in addons-linter should be cleared, merged and released.

Once the update addons-linter PR is merged, everything in this PR should be good to go too.

kumar303

Thanks for the quick turnaround. I requested some minor test cleanup.

kumar303 · 2016-10-31T21:56:53Z

tests/unit/test-cmd/test.lint.js

@@ -96,13 +104,16 @@ describe('lint', () => {
      boring: 'boring flag',
      // $FLOW_IGNORE: wrong type used for testing purpose
      selfHosted: 'self-hosted flag',
+      // $FLOW_IGNORE: wrong type used for testing purpose
+      warningsAsErrors: 'warnings-as-errors flag',


I think you can remove this entirely. Or is there any reason to include it? Do the tests fail?

kumar303 · 2016-10-31T21:57:08Z

tests/unit/test-cmd/test.lint.js

    }).then(() => {
      const config = createLinter.firstCall.args[0].config;
      assert.equal(config.pretty, 'pretty flag');
      assert.equal(config.metadata, 'metadata flag');
      assert.equal(config.output, 'output value');
      assert.equal(config.boring, 'boring flag');
      assert.equal(config.selfHosted, 'self-hosted flag');
+      assert.equal(config.warningsAsErrors, 'warnings-as-errors flag');


you can remove this too (unless it's needed?)

josteink · 2016-11-02T20:31:20Z

I've now fixed up everything as you've asked me to, and added a -w shorthand parameter for the feature as well. Still... We need addons-linter fixed before this PR is going anywhere.

And nothing seems to be happening there. Should I (or someone) file a PR to revert the yargs-changes causing this breakage?

coveralls · 2016-11-02T20:34:55Z

Coverage remained the same at 100.0% when pulling 4ded9b9 on josteink:warnings-as-errors into 36989b4 on mozilla:master.

kumar303

Thanks for all the changes

kumar303 · 2016-11-04T20:22:41Z

We need addons-linter fixed before this PR is going anywhere.

Yeah, I'll wait on mozilla/addons-linter#1023 before merging

Should I (or someone) file a PR to revert the yargs-changes causing this breakage?

I haven't had a chance to look at it. @tofumatt have you had time to take a look?

If you can see anything in the yargs changelog that stands out, let us know. I'm not sure if it's a yargs regression or not. I suspect it has something to do with how web-ext uses the linter programmatically and how both programs use yargs. We fixed one issue related to this already. It was that the linter was trying to parse the command line input a second time.

tofumatt · 2016-11-04T20:26:24Z

I haven't figured it out yet though I've looked a bit. I'll have another
look tonight or this weekend if I can though!

tofumatt

On 4 November 2016 at 20:22:42, Kumar McMillan (notifications@github.com)
wrote:

We need addons-linter fixed before this PR is going anywhere.

Yeah, I'll wait on mozilla/addons-linter#1023
mozilla/addons-linter#1023 before merging

Should I (or someone) file a PR to revert the yargs-changes causing this
breakage?

I haven't had a chance to look at it. @tofumatt
https://github.com/tofumatt have you had time to take a look?

If you can see anything in the yargs changelog that stands out, let us
know. I'm not sure if it's a yargs regression or not. I suspect it has
something to do with how web-ext uses the linter programmatically
https://github.com/mozilla/web-ext/blob/master/src/cmd/lint.js#L63 and
how both programs use yargs. We fixed one issue related to this already. It
was that the linter was trying to parse the command line input a second
time.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#613 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAFi9573V6hPfa1pJygr2s6Aj75mzKVlks5q65QSgaJpZM4KhkNP
.

josteink · 2016-11-04T20:46:45Z

I've just taken a cursory glance at this issue, and it didn't get anywhere.

I do have quite limited time at hand these days, so if you could dig deeper into this, that would really be appreciated.

coveralls · 2016-11-07T19:55:08Z

Coverage remained the same at 100.0% when pulling 4eee665 on josteink:warnings-as-errors into 5a4046b on mozilla:master.

This patch adds support for using --warnings-as-errors as a parameter for the lint command. This complements the PR mozilla/addons-linter#1016 and completes the issue mozilla/addons-linter#1014.

josteink · 2016-11-08T09:50:52Z

Just tested this patch with latest version of addons-linter (0.15.10) and everything now works as it should :)

This PR should now be ready for merge.

coveralls · 2016-11-08T09:56:05Z

Coverage remained the same at 100.0% when pulling 5019374 on josteink:warnings-as-errors into 93b55f0 on mozilla:master.

kumar303 · 2016-11-08T15:33:01Z

Yep, I bumped our linter dep with #632

kumar303 · 2016-11-08T15:33:31Z

Thanks for all your work on this feature!

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

josteink force-pushed the warnings-as-errors branch from 83b1443 to 16103e9 Compare October 26, 2016 19:36

josteink force-pushed the warnings-as-errors branch from 16103e9 to d9db669 Compare October 28, 2016 17:40

kumar303 suggested changes Oct 29, 2016

View reviewed changes

josteink force-pushed the warnings-as-errors branch from d9db669 to 446aa8f Compare October 29, 2016 18:54

kumar303 suggested changes Oct 30, 2016

View reviewed changes

josteink force-pushed the warnings-as-errors branch from 446aa8f to b7460cc Compare October 30, 2016 19:12

kumar303 suggested changes Oct 31, 2016

View reviewed changes

josteink force-pushed the warnings-as-errors branch from b7460cc to 4ded9b9 Compare November 2, 2016 20:29

kumar303 approved these changes Nov 4, 2016

View reviewed changes

josteink force-pushed the warnings-as-errors branch from 4ded9b9 to 4eee665 Compare November 7, 2016 19:49

josteink added 2 commits November 8, 2016 10:47

feat: Support warnings-as-errors

47f9e21

This patch adds support for using --warnings-as-errors as a parameter for the lint command. This complements the PR mozilla/addons-linter#1016 and completes the issue mozilla/addons-linter#1014.

chore: Update addons-linter to requirered minimum-version.

5019374

josteink force-pushed the warnings-as-errors branch from 4eee665 to 5019374 Compare November 8, 2016 09:50

kumar303 merged commit ec7a1ee into mozilla:master Nov 8, 2016

kumar303 removed the state: pull request ready label Nov 8, 2016

This was referenced Nov 8, 2016

Add lint option: --warnings-as-errors #633

Closed

web-ext lint --warnings-as-errors has no effect #634

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support warnings-as-errors #613

Support warnings-as-errors #613

josteink commented Oct 26, 2016 •

edited

coveralls commented Oct 26, 2016 •

edited

coveralls commented Oct 28, 2016 •

edited

kumar303 left a comment

josteink commented Oct 29, 2016

coveralls commented Oct 29, 2016

kumar303 commented Oct 30, 2016

kumar303 left a comment

kumar303 Oct 30, 2016

kumar303 Oct 31, 2016

kumar303 Oct 30, 2016

josteink commented Oct 30, 2016

josteink commented Oct 30, 2016

kumar303 commented Oct 30, 2016

coveralls commented Oct 30, 2016 •

edited

josteink commented Oct 31, 2016

kumar303 left a comment

kumar303 Oct 31, 2016

kumar303 Oct 31, 2016

josteink commented Nov 2, 2016

coveralls commented Nov 2, 2016 •

edited

kumar303 left a comment

kumar303 commented Nov 4, 2016

tofumatt commented Nov 4, 2016

josteink commented Nov 4, 2016

coveralls commented Nov 7, 2016 •

edited

josteink commented Nov 8, 2016

coveralls commented Nov 8, 2016 •

edited

kumar303 commented Nov 8, 2016

kumar303 commented Nov 8, 2016

Support warnings-as-errors #613

Support warnings-as-errors #613

Conversation

josteink commented Oct 26, 2016 • edited

coveralls commented Oct 26, 2016 • edited

coveralls commented Oct 28, 2016 • edited

kumar303 left a comment

Choose a reason for hiding this comment

josteink commented Oct 29, 2016

coveralls commented Oct 29, 2016

kumar303 commented Oct 30, 2016

kumar303 left a comment

Choose a reason for hiding this comment

kumar303 Oct 30, 2016

Choose a reason for hiding this comment

kumar303 Oct 31, 2016

Choose a reason for hiding this comment

kumar303 Oct 30, 2016

Choose a reason for hiding this comment

josteink commented Oct 30, 2016

josteink commented Oct 30, 2016

kumar303 commented Oct 30, 2016

coveralls commented Oct 30, 2016 • edited

josteink commented Oct 31, 2016

kumar303 left a comment

Choose a reason for hiding this comment

kumar303 Oct 31, 2016

Choose a reason for hiding this comment

kumar303 Oct 31, 2016

Choose a reason for hiding this comment

josteink commented Nov 2, 2016

coveralls commented Nov 2, 2016 • edited

kumar303 left a comment

Choose a reason for hiding this comment

kumar303 commented Nov 4, 2016

tofumatt commented Nov 4, 2016

josteink commented Nov 4, 2016

coveralls commented Nov 7, 2016 • edited

josteink commented Nov 8, 2016

coveralls commented Nov 8, 2016 • edited

kumar303 commented Nov 8, 2016

kumar303 commented Nov 8, 2016

josteink commented Oct 26, 2016 •

edited

coveralls commented Oct 26, 2016 •

edited

coveralls commented Oct 28, 2016 •

edited

coveralls commented Oct 30, 2016 •

edited

coveralls commented Nov 2, 2016 •

edited

coveralls commented Nov 7, 2016 •

edited

coveralls commented Nov 8, 2016 •

edited