Update: More detailed assert message for rule-tester #9769

starkwang · 2017-12-25T07:34:57Z

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[X] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)
The assert message now is a little bit confused. It only tells you "Output is incorrect.", but doesn't specificly show the received value and the expected value.

This change is to make the assert message more detailed.

Is there anything you'd like reviewers to focus on?
Not yet.

not-an-aardvark · 2017-12-25T07:54:50Z

Thanks for the pull request!

This was actually proposed before in #8173. I think the reason you're seeing confusing error messages is because you're using RuleTester without a test runner. For example, when using mocha, the error message looks like this:

This is because mocha creates a diff based on the actual and expected properties of the AssertionError objects.

I don't think it would make sense to have the error message change for people using mocha, since it already ends up fairly descriptive. However, I agree that it would be a good idea to improve the error message in the case where no test runner is being used (by looking at the actual and expected properties like mocha does). Would you be able to update this PR to just change that case instead?

This function gets called when no test runner is active. (The first argument is the name of the test case, and the second argument is a function that may or may not throw an AssertionError.) I think this could be fixed by adding a try-catch to that function, and rethrowing any thrown AssertionError with a more descriptive message.

starkwang · 2017-12-25T08:48:38Z

@not-an-aardvark Thanks for your comment!
I've just updated this PR to address comment.

platinumazure

Could you please add or modify a unit test demonstrating the changes (lack of message, explicitly check for actual/expected/operator, etc.)? Thanks!

starkwang · 2017-12-25T14:51:42Z

@platinumazure Thanks for review.

This change is only for the case where no test runner is being used. But our unit tests seem based on some runners (hope I don't miss something), so it actually doesn't break any unit test now.

Is there any test for the no-test-runner case to add in?

platinumazure · 2017-12-25T14:59:07Z

Are there any tests for RuleTester.describe and RuleTester.it now, with them being called directly rather than overwritten? If so, it seems like those would be a good place to test. If there aren't tests (possible since the default behavior is to proxy into the method passed in), we should add some.

not-an-aardvark · 2017-12-25T19:41:04Z

lib/testers/rule-tester.js

+            actual: e.actual,
+            expected: e.expected,
+            operator: e.operator
+        });


I had been envisioning something more like this:

try { return method.apply(this); } catch (err) { if (err instanceof assert.AssertionError) { throw Object.assign({}, new assert.AssertionError, { message: `${e.actual} ${e.operator} ${e.expected}` }) } throw err; }

That way, any unrelated errors (e.g. crashes in a rule) would still get passed through unmodified, and the original message (e.g. "output is incorrect") would still be present for additional context.

@not-an-aardvark using a plain object as error object is not the same as throwing a new AssertionError. Using Object.assign is also not sufficient because stack and message are non-enumerable and would not be copied.

Oops, you're right; I had forgotten that the thrown object would no longer be an AssertionError. I think something like this could work:

try { return method.apply(this); } catch (err) { if (err instanceof assert.AssertionError) { err.message += `${util.inspect(err.actual)} ${err.operator} ${util.inspect(err.expected)}`; } throw err; }

To me, the important constraints are:

We don't add redundant info in the case where a test runner is being used (this is by far the most common case)

The error message still contains information about what the problem is (e.g. it should still say something like "output is incorrect" when the output is incorrect, rather than just displaying a diff between two strings)

not-an-aardvark · 2017-12-25T19:42:24Z

@platinumazure I agree that it would be good to have tests like that, although I don't know how we would actually create them. RuleTester selects a behavior based on the presence of describe and it globals, and those globals are already present when we are running our tests.

j-f1 · 2017-12-25T21:21:28Z

You could remove the globals, require RuleTester, then reinstate them.

starkwang · 2017-12-26T14:07:42Z

Pushed commit to address comment.
@not-an-aardvark @platinumazure Please take another review : )

platinumazure

LGTM, just one small thing but not a blocker at all. Thanks for adding a test, as well.

platinumazure · 2017-12-26T14:22:05Z

tests/lib/testers/no-test-runners.js

+it = null;
+describe = null;
+
+const ruleTester = new RuleTester();


I'd feel better if this logic (from here to the end of the assertion) were wrapped in a try/finally so that the globals are definitely reassigned even if an exception is thrown.

I've added the try/finally : )

platinumazure

LGTM! Note that you could omit the catch clause (try/finally are legal without catch. A try statement just needs at least one catch or finally).

not-an-aardvark · 2017-12-26T20:00:51Z

lib/testers/rule-tester.js

+        return method.apply(this);
+    } catch (err) {
+        if (err instanceof assert.AssertionError) {
+            err.message = `${util.inspect(err.actual)} ${err.operator} ${util.inspect(err.expected)}`;


Can you make this append to the existing message rather than overwriting it? I think it would be good to retain the "Output is incorrect" text to make the source of the error clearer.

Oh, I forgot to explain here.
The defaultHandler is called nestedly somehow (sorry I don't dive into the code but its behaviour shows that), so the thrown error will be caught and err message
will be appended in multiple times. It will finally become like

"Output is incorrect 'a' == 'b' 'a' == 'b' 'a' == 'b'"

That's the reason for overwriting it here.

Another solution is adding a flag like appended on error object to avoid multiple appending. But is it worth to do it?

Oh, I see. I think that's happening because defaultHandler is used for both the "describe" and the "it" methods. So effectively something like this is happening:

defaultHandler("foo", () => { defaultHandler("bar", () => { // throw assertion error }); });

One solution would be to use the old default handler (which just calls the function in the second argument) as the default for describe, and use the handler that appends a diff as the default for it.

starkwang · 2017-12-27T02:13:11Z

Actually I have another idea to implement this (I think this is better):

This is the code we using now:

assert.equal(fixResult.output, item.output, "Output is incorrect.");

We can change it into:

const message = "Output is incorrect."
const inTestRunner = typeof describe === "function" || typeof it === "function"
if (!inTestRunner) {
    // if not using test runner, the assert message should be appended by diff message.
    message += ` Expected: ${item.output} Received: ${fixResult.output}`
}
assert.equal(fixResult.output, item.output, message);

@not-an-aardvark What do you think about this? : )

platinumazure · 2017-12-27T02:19:27Z

I think a flag (maybe even a symbol property?) would be better here. If I develop a plugin and choose to use a test framework that doesn't export those globals (and I overwrite RuleTester.describe and RuleTester.it), then the latest suggestion would incorrectly infer I'm never in a test environment. Thanks for bearing with us as we keep working towards a solution. I really appreciate your patience and persistence.

…

On Dec 26, 2017 8:13 PM, "Weijia Wang" ***@***.***> wrote: Actually I have another idea to implement this (I think this is better): This is the code we using now: assert.equal(fixResult.output, item.output, "Output is incorrect."); We can change it into: const errMsg = "Output is incorrect."const inTestRunner = typeof describe === "function" || typeof it === "function"if (!inTestRunner) { // if not using test runner, the error message should be appended by diff message. errMsg += ` Expected: ${item.output} Received: ${fixResult.output}` }assert.equal(fixResult.output, item.output, errMsg); @not-an-aardvark <https://github.com/not-an-aardvark> What do you think about this? : ) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9769 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AARWerYekjDKrEH3mmfjuEalA5zSx3Shks5tEae4gaJpZM4RMKX_> .

starkwang · 2017-12-27T03:19:35Z

Updated PR to address comment. Please take another look. 😀

not-an-aardvark

LGTM, thanks!

platinumazure

LGTM except for one small issue with the exception message. Thanks!

platinumazure · 2017-12-29T14:09:19Z

lib/testers/rule-tester.js

+        return method.apply(this);
+    } catch (err) {
+        if (err instanceof assert.AssertionError) {
+            err.message += ` ${util.inspect(err.actual)} ${err.operator} ${util.inspect(err.expected)}`;


I think surrounding the comparison in parentheses would be useful and make the error message more readable: (${util.inspect(err.actual)} ${err.operator} ${util.inspect(err.expected)})

Thanks for review. I've added the parentheses :-)

platinumazure

LGTM, thanks!

ilyavolodin

Thanks for PR

starkwang mentioned this pull request Dec 25, 2017

tools: auto fix custom eslint rule for lowercase-name-for-primitive.js nodejs/node#17715

Closed

3 tasks

starkwang force-pushed the detailed-assert-msg branch 2 times, most recently from 549e40a to 7cfa106 Compare December 25, 2017 08:48

platinumazure requested changes Dec 25, 2017

View reviewed changes

not-an-aardvark reviewed Dec 25, 2017

View reviewed changes

Update: More detailed assert message for rule-tester

7252351

starkwang force-pushed the detailed-assert-msg branch from 7cfa106 to 7252351 Compare December 26, 2017 14:07

platinumazure approved these changes Dec 26, 2017

View reviewed changes

starkwang force-pushed the detailed-assert-msg branch from fa2a5d3 to 4160e75 Compare December 26, 2017 14:32

[squash]add try/finally block

a3f60f0

starkwang force-pushed the detailed-assert-msg branch from 4160e75 to a3f60f0 Compare December 26, 2017 14:34

platinumazure approved these changes Dec 26, 2017

View reviewed changes

not-an-aardvark suggested changes Dec 26, 2017

View reviewed changes

[squash]append message instead of overwriting it

2d33618

not-an-aardvark approved these changes Dec 27, 2017

View reviewed changes

platinumazure requested changes Dec 29, 2017

View reviewed changes

[squash]add parentheses surrounding error message

f17d679

platinumazure approved these changes Dec 29, 2017

View reviewed changes

ilyavolodin approved these changes Jan 4, 2018

View reviewed changes

ilyavolodin merged commit c64195f into eslint:master Jan 4, 2018

renovate bot mentioned this pull request Feb 14, 2018

Update dependency eslint to v4 Urigo/tortilla#62

Merged

eslint-deprecated bot locked and limited conversation to collaborators Jul 4, 2018

eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Jul 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update: More detailed assert message for rule-tester #9769

Update: More detailed assert message for rule-tester #9769

starkwang commented Dec 25, 2017

not-an-aardvark commented Dec 25, 2017

starkwang commented Dec 25, 2017

platinumazure left a comment

starkwang commented Dec 25, 2017 •

edited

platinumazure commented Dec 25, 2017

not-an-aardvark Dec 25, 2017

BridgeAR Dec 26, 2017 •

edited

not-an-aardvark Dec 26, 2017

not-an-aardvark commented Dec 25, 2017

j-f1 commented Dec 25, 2017

starkwang commented Dec 26, 2017

platinumazure left a comment

platinumazure Dec 26, 2017

starkwang Dec 26, 2017

platinumazure left a comment

not-an-aardvark Dec 26, 2017

starkwang Dec 27, 2017

not-an-aardvark Dec 27, 2017

starkwang commented Dec 27, 2017 •

edited

platinumazure commented Dec 27, 2017 via email

starkwang commented Dec 27, 2017

not-an-aardvark left a comment

platinumazure left a comment

platinumazure Dec 29, 2017

starkwang Dec 29, 2017

platinumazure left a comment

ilyavolodin left a comment

Update: More detailed assert message for rule-tester #9769

Update: More detailed assert message for rule-tester #9769

Conversation

starkwang commented Dec 25, 2017

not-an-aardvark commented Dec 25, 2017

starkwang commented Dec 25, 2017

platinumazure left a comment

Choose a reason for hiding this comment

starkwang commented Dec 25, 2017 • edited

platinumazure commented Dec 25, 2017

Choose a reason for hiding this comment

BridgeAR Dec 26, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

not-an-aardvark commented Dec 25, 2017

j-f1 commented Dec 25, 2017

starkwang commented Dec 26, 2017

platinumazure left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

platinumazure 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

starkwang commented Dec 27, 2017 • edited

platinumazure commented Dec 27, 2017 via email

starkwang commented Dec 27, 2017

not-an-aardvark left a comment

Choose a reason for hiding this comment

platinumazure left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

platinumazure left a comment

Choose a reason for hiding this comment

ilyavolodin left a comment

Choose a reason for hiding this comment

starkwang commented Dec 25, 2017 •

edited

BridgeAR Dec 26, 2017 •

edited

starkwang commented Dec 27, 2017 •

edited