Skip to content

Conversation

@SamuelMarks
Copy link
Contributor

Changed new Error object tests to check .message; so tests pass once more.

@coveralls
Copy link

coveralls commented Jul 8, 2017

Coverage Status

Coverage remained the same at 98.174% when pulling ec1bbeb on SamuelMarks:master into a759b94 on restify:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 98.174% when pulling ec1bbeb on SamuelMarks:master into a759b94 on restify:master.

@simonepri
Copy link

LGTM
@retrohacker can we merge this?

@simonepri
Copy link

@SamuelMarks could you list all the other PRs that this will close?
I mean the list of PRs made from @greenkeeperio-bot that could be closed after the merge of this PR.
Thanks 👍

@SamuelMarks
Copy link
Contributor Author

Closes:
#35 #44 #45 #47 #48 #56 #57 #58 #59 #60 #65 #66 #67 #68 #69 #70

@DonutEspresso
Copy link
Member

Thanks @SamuelMarks! Can you provide some context on the changes to the tests? I'm curious which updated dependency caused them to fail?

@simonepri
Copy link

Any news about that?

@SamuelMarks
Copy link
Contributor Author

I'm not sure @DonutEspresso; I tend to just update all dependencies and tests until they all pass.

@DonutEspresso
Copy link
Member

@SamuelMarks I brought your PR down locally - I think the breakage is actually due to a bug in the existing tests. The tests were doing a deepEqual but somehow chai was passing it, but chai@4.x must be stricter (and arguably correct, IMHO). The updates to the tests you made can be fixed by just applying the same fix to both the two failing tests:

-            var parsed = parse(new Error('foobar'), options);
+            var parsed = parse(err, options);

It appeared the existing tests were doing a deepEqual but actually passing an incorrect reference. Let me know if you have the time to make the changes! Otherwise, I can take your work here and run with it. 👍 Appreciate the help!

@simonepri
Copy link

@DonutEspresso It's a good idea to commit the package-lock file?

@simonepri
Copy link

simonepri commented Aug 16, 2017

@SamuelMarks @DonutEspresso I've opened a PR with the fix:
https://github.com/SamuelMarks/errors/pull/1
Tests are passing now

@DonutEspresso
Copy link
Member

@simonepri Looks great! If you are able to open up a PR against this repo, I can merge it in.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.008%) to 98.182% when pulling 2617887 on SamuelMarks:master into a759b94 on restify:master.

1 similar comment
@coveralls
Copy link

coveralls commented Sep 3, 2017

Coverage Status

Coverage increased (+0.008%) to 98.182% when pulling 2617887 on SamuelMarks:master into a759b94 on restify:master.

@DonutEspresso
Copy link
Member

Thank you @SamuelMarks @simonepri! Appreciate the help in getting this over the finish line. Apologies for this taking so long!

@DonutEspresso DonutEspresso merged commit ebe71cc into restify:master Sep 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants