fix: Added errno for errors with specific codes #488

rackstar17 · 2016-09-15T02:07:30Z

Fixes #220

coveralls · 2016-09-15T02:12:18Z

Coverage decreased (-8.6%) to 91.429% when pulling 7d4c0e6 on rackstar17:fix/220 into 0d1b824 on mozilla:master.

kumar303 · 2016-09-15T15:36:47Z

Hi. Thanks for getting this started!

I think it would be easier to start this by first adding a test that expects errno to be caught. You could model the test on this one:

web-ext/tests/unit/test.errors.js

Line 43 in ee197c4

it('catches errors having a code', () => {

The test suite output is going to be messy when you work on this feature because the function, onlyErrorsWithCode(), is used by many other tests. To hide the irrelevant failures, you can run only the tests for this specific function like this:

./node_modules/.bin/mocha -r babel-core/register tests/unit/test.errors.js

coveralls · 2016-09-18T21:47:40Z

Coverage remained the same at 100.0% when pulling 7d2e6b8 on rackstar17:fix/220 into 0d1b824 on mozilla:master.

rackstar17 · 2016-09-18T23:25:38Z

@kumar303 Hi. I tried to do what you said but I can't resolve the travis build issues
I also tried to run the test.errors.js file as you mentioned and locally all the tests were passed for test.errors.js . Can you help me with it ?

coveralls · 2016-09-19T14:05:09Z

Coverage decreased (-0.2%) to 99.838% when pulling d62b2a5 on rackstar17:fix/220 into cb50f9f on mozilla:master.

coveralls · 2016-09-19T14:20:23Z

Coverage remained the same at 100.0% when pulling d330f9e on rackstar17:fix/220 into cb50f9f on mozilla:master.

kumar303 · 2016-09-19T20:16:33Z

The test failure may have been a timeout caused by slowness in the TravisCI system, not with your patch. I have restarted the test to see if it still fails repeatedly.

kumar303

This is looking really good, thanks for following up.

You also need to add a test that checks for errno in an array of codes. You can model that test after this one. Because you used a second if for the errno Array check (instead of an else), the code coverage checker did not catch this omission -- tricky!

kumar303 · 2016-09-19T20:18:32Z

src/errors.js

+      if (codeWanted.indexOf(error.code) !== -1) {
+        throwError = false;
+      }
+      if (codeWanted.indexOf(error.errno) !== -1) {


You can group these statements to make the code more concise and less repetitive. Example:

if (codeWanted.indexOf(error.code) !== -1 || codeWanted.indexOf(error.errno) !== -1) { throwError = false; }

You could possibly even use Array#includes(), which I think is supported in Firefox 43+.

NodeJS is our target platform with 4.0 being the minimum. I just checked and sadly 4.0 doesn't have this function.

I added the || code to reduce the repetitiveness of if statements but there was this issue of not having more than 80 columns in a single line.
It could be done this way, didn't think of that earlier .
Done.

kumar303 · 2016-09-19T20:21:53Z

tests/unit/test.errors.js

@@ -34,9 +34,11 @@ describe('errors', () => {

    class ErrorWithCode extends Error {
      code: string;
+      errno: string;


instead of editing this class, I think you need to create a new class named something like ErrorWithErrno and add errno to that one only. Otherwise, the existing tests for error codes will not be as effective.

kumar303 · 2016-09-19T20:28:43Z

... may have been a timeout caused by slowness in the TravisCI system

Yes, this was it. I restarted the build and the tests are passing now. Sorry for the confusion! This doesn't happen too often but it's a known problem with this test.

coveralls · 2016-09-19T22:36:29Z

Coverage remained the same at 100.0% when pulling b5a8f00 on rackstar17:fix/220 into cb50f9f on mozilla:master.

kumar303

This is really close, thanks for the follow-ups. I requested some changes to make the tests more realistic.

kumar303 · 2016-09-21T22:04:21Z

tests/unit/test.errors.js

@@ -40,13 +40,28 @@ describe('errors', () => {
      }
    }

+    class ErrorWithErrno extends Error {
+      errno: string;


this should be errno: number

kumar303 · 2016-09-21T22:05:01Z

tests/unit/test.errors.js

+      errno: string;
+      constructor() {
+        super('pretend this is a system error');
+        this.errno = 'SOME_CODE';


this should be something like this.errno = 53, which means ENOTEMPTY

kumar303 · 2016-09-21T22:05:54Z

tests/unit/test.errors.js

    it('catches errors having a code', () => {
      return Promise.reject(new ErrorWithCode())
        .catch(onlyErrorsWithCode('SOME_CODE', (error) => {
          assert.equal(error.code, 'SOME_CODE');
        }));
    });

+    it('catches errors having a error no', () => {
+      return Promise.reject(new ErrorWithErrno())
+        .catch(onlyErrorsWithCode('SOME_CODE', (error) => {


and then this would need to be onlyErrorsWithCode(53, ...)

kumar303 · 2016-09-21T22:06:17Z

tests/unit/test.errors.js

@@ -65,6 +80,13 @@ describe('errors', () => {
        }));
    });

+    it('catches errors having one of many errno', () => {
+      return Promise.reject(new ErrorWithErrno())
+        .catch(onlyErrorsWithCode(['OTHER_CODE', 'SOME_CODE'], (error) => {


and this would need updating too

kumar303 · 2016-09-21T22:08:01Z

tests/unit/test.errors.js

@@ -76,6 +98,17 @@ describe('errors', () => {
        });
    });

+    it('throws errors that are not in an array of errno', () => {


I don't think you need to include this test. It doesn't introduce any new coverage since the previous one already checks that an array of mismatching codes does not catch any error at all.

rackstar17 · 2016-09-21T22:28:50Z

@kumar303 Hi, the travis checks have failed . Can you take a look ?
Thanks.

rackstar17 · 2016-09-21T22:31:21Z

Ah , I see now .
The thing you suggested to make errno: number , the onlyErrorsWithCode expects it's parameter to be string or array of strings and here we are passing number .

kumar303 · 2016-09-21T22:33:59Z

Yes, exactly, we use Flow to incorporate static typing into the code. You'll need to specify that codeWanted can take an array of either strings or numbers. It's a bit cryptic due to polymorphism so here is the typing fix:

codeWanted: string | Array<string|number>

coveralls · 2016-09-21T22:49:46Z

Coverage remained the same at 100.0% when pulling b4fe6f2 on rackstar17:fix/220 into da5e0f0 on mozilla:master.

rackstar17 · 2016-09-25T01:49:48Z

@kumar303 Hi,
The tests have passed , Can you review it ?
Thanks .

kumar303

Looks great, thanks for all the follow-ups

atsay · 2016-09-26T13:49:37Z

Thanks for your contribution, @rackstar17! I've added it to our recognition page.

fix: Added errno for errors with specific codes

7d4c0e6

muffinresearch added the state: pull request ready label Sep 15, 2016

rackstar17 force-pushed the fix/220 branch from 7d2e6b8 to d62b2a5 Compare September 19, 2016 13:54

fix: Added test for errno

d330f9e

rackstar17 force-pushed the fix/220 branch from d62b2a5 to d330f9e Compare September 19, 2016 14:09

kumar303 suggested changes Sep 19, 2016

View reviewed changes

fix: Added test for errno and addressed review comments

b5a8f00

kumar303 suggested changes Sep 21, 2016

View reviewed changes

rackstar17 force-pushed the fix/220 branch from 57d9881 to 866d839 Compare September 21, 2016 22:38

fix: Addressed review comments

b4fe6f2

rackstar17 force-pushed the fix/220 branch from 866d839 to b4fe6f2 Compare September 21, 2016 22:43

kumar303 approved these changes Sep 25, 2016

View reviewed changes

kumar303 self-assigned this Sep 25, 2016

kumar303 merged commit 0d4141b into mozilla:master Sep 25, 2016

kumar303 removed the state: pull request ready label Sep 25, 2016

rackstar17 deleted the fix/220 branch September 26, 2016 18:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Added errno for errors with specific codes #488

fix: Added errno for errors with specific codes #488

rackstar17 commented Sep 15, 2016 •

edited by kumar303

coveralls commented Sep 15, 2016 •

edited

kumar303 commented Sep 15, 2016

coveralls commented Sep 18, 2016 •

edited

rackstar17 commented Sep 18, 2016

coveralls commented Sep 19, 2016 •

edited

coveralls commented Sep 19, 2016 •

edited

kumar303 commented Sep 19, 2016

kumar303 left a comment

kumar303 Sep 19, 2016

pdehaan Sep 19, 2016

kumar303 Sep 19, 2016

rackstar17 Sep 19, 2016

kumar303 Sep 19, 2016 •

edited

rackstar17 Sep 19, 2016

kumar303 commented Sep 19, 2016

coveralls commented Sep 19, 2016 •

edited

kumar303 left a comment

kumar303 Sep 21, 2016

rackstar17 Sep 21, 2016

kumar303 Sep 21, 2016

rackstar17 Sep 21, 2016

kumar303 Sep 21, 2016

rackstar17 Sep 21, 2016

kumar303 Sep 21, 2016

rackstar17 Sep 21, 2016

kumar303 Sep 21, 2016

rackstar17 Sep 21, 2016

rackstar17 commented Sep 21, 2016

rackstar17 commented Sep 21, 2016

kumar303 commented Sep 21, 2016

coveralls commented Sep 21, 2016 •

edited

rackstar17 commented Sep 25, 2016

kumar303 left a comment

atsay commented Sep 26, 2016

fix: Added errno for errors with specific codes #488

fix: Added errno for errors with specific codes #488

Conversation

rackstar17 commented Sep 15, 2016 • edited by kumar303

coveralls commented Sep 15, 2016 • edited

kumar303 commented Sep 15, 2016

coveralls commented Sep 18, 2016 • edited

rackstar17 commented Sep 18, 2016

coveralls commented Sep 19, 2016 • edited

coveralls commented Sep 19, 2016 • edited

kumar303 commented Sep 19, 2016

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

kumar303 Sep 19, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumar303 commented Sep 19, 2016

coveralls commented Sep 19, 2016 • edited

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

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

rackstar17 commented Sep 21, 2016

rackstar17 commented Sep 21, 2016

kumar303 commented Sep 21, 2016

coveralls commented Sep 21, 2016 • edited

rackstar17 commented Sep 25, 2016

kumar303 left a comment

Choose a reason for hiding this comment

atsay commented Sep 26, 2016

rackstar17 commented Sep 15, 2016 •

edited by kumar303

coveralls commented Sep 15, 2016 •

edited

coveralls commented Sep 18, 2016 •

edited

coveralls commented Sep 19, 2016 •

edited

coveralls commented Sep 19, 2016 •

edited

kumar303 Sep 19, 2016 •

edited

coveralls commented Sep 19, 2016 •

edited

coveralls commented Sep 21, 2016 •

edited