Improve "Error type should be" assertion in rule-tester

[DrewML]

This issue is very similar to #5917, but it's about a different assertion message.

When using the type property in an errors object for invalid code, and the test value does not match the expected value, the following message is currently spit to stdout:

     AssertionError: Error type should be CallExpression
      at testInvalidTemplate (lib/testers/rule-tester.js:9:12161)
      at Context.<anonymous> (lib/testers/rule-tester.js:9:14634)

For debugging purposes, it would be helpful to know what value was provided for the type, not just the expected value.

You can replicate the existing behavior with the following:

Use this descriptor for invalid code in a rule-tester run

    {
        code: "put some code that will fail",
        errors: [{
            line: 1,
            column: 5,
            message: "Some message",
            type: "CallExpression"
        }]
    }

Use this code in a basic rule

context.report({
   // Note that we're not passing a `node` property to `report`
   message: "Some Message",
   loc: {line: 0, column: 0}
});

The source of this problem is in /lib/testers/rule-tester.js on line 400:

assert.equal(messages[i].nodeType, item.errors[i].type, "Error type should be " + item.errors[i].type);

Augmenting the assertion message to include the value for messages[i].nodeType would make debugging a bit easier.

Is there any interest in a PR to improve this?

[alberto]

@nzakas Would changing the message be a breaking change? Asking because you mentioned it in #6084

[ilyavolodin]

@alberto This is changing message in rule-tester only, so it will only affect developers of the plugins + us. So pretty sure this will be non-breaking.
@DrewML I think ideally we would want to changing this so you don't get stack trace at all (since it's pretty useless) and instead get before/after. Problem is, most of as are already getting before/after instead of stack trace, and we haven't been able to figure out why.

[DrewML]

Problem is, most of as are already getting before/after instead of stack trace, and we haven't been able to figure out why.

@ilyavolodin I do get the correct output from most of the assertions, but this particular one seems to be an issue. Can you check whether you see the before/after or a stack trace when using the example I posted above?

[nzakas]

@alberto not breaking. #6084 changed the type of asserting being done, this is just changing the message.

What I don't understand if why the actual and expected aren't being output already. I thought Chai did that when using comparisons by default?

[DrewML]

@nzakas I don't think Chai's assert API is being used here. I believe the behavior we're seeing is just the behavior of node's built in assert module when a message is passed.

Example:

> var assert = require("assert");
undefined
> assert.equal(1,2)
AssertionError: 1 == 2
    at repl:1:8
    at REPLServer.defaultEval (repl.js:272:27)
    at bound (domain.js:280:14)
    at REPLServer.runBound [as eval] (domain.js:293:12)
    at REPLServer.<anonymous> (repl.js:441:10)
    at emitOne (events.js:101:20)
    at REPLServer.emit (events.js:188:7)
    at REPLServer.Interface._onLine (readline.js:219:10)
    at REPLServer.Interface._line (readline.js:561:8)
    at REPLServer.Interface._ttyWrite (readline.js:838:14)
> assert.equal(1,2, "message")
AssertionError: message
    at repl:1:8
    at REPLServer.defaultEval (repl.js:272:27)
    at bound (domain.js:280:14)
    at REPLServer.runBound [as eval] (domain.js:293:12)
    at REPLServer.<anonymous> (repl.js:441:10)
    at emitOne (events.js:101:20)
    at REPLServer.emit (events.js:188:7)
    at REPLServer.Interface._onLine (readline.js:219:10)
    at REPLServer.Interface._line (readline.js:561:8)
    at REPLServer.Interface._ttyWrite (readline.js:838:14)

I'm guessing Chai doesn't instrument the code before a run, so it probably only sees whatever the assert API spits out.

[nzakas]

@DrewML ah, good call. I forgot we weren't using Chai.

I'm 👍 For this.

[DrewML]

Cool - I'll start working on this tonight. Safe to assume we want to do this for all the assertions in that same area?

For reference:

            if (typeof item.errors === "number") {
                assert.equal(messages.length, item.errors, util.format("Should have %d error%s but had %d: %s",
                    item.errors, item.errors === 1 ? "" : "s", messages.length, util.inspect(messages)));
            } else {
                assert.equal(messages.length, item.errors.length,
                    util.format("Should have %d error%s but had %d: %s",
                    item.errors.length, item.errors.length === 1 ? "" : "s", messages.length, util.inspect(messages)));

                for (var i = 0, l = item.errors.length; i < l; i++) {
                    assert.ok(!("fatal" in messages[i]), "A fatal parsing error occurred: " + messages[i].message);
                    assert.equal(messages[i].ruleId, ruleName, "Error rule name should be the same as the name of the rule being tested");

                    if (typeof item.errors[i] === "string") {

                        // Just an error message.
                        assert.equal(messages[i].message, item.errors[i], "Error message should be " + item.errors[i]);
                    } else if (typeof item.errors[i] === "object") {

                        /*
                         * Error object.
                         * This may have a message, node type, line, and/or
                         * column.
                         */
                        if (item.errors[i].message) {
                            assert.equal(messages[i].message, item.errors[i].message, "Error message should be " + item.errors[i].message);
                        }

                        if (item.errors[i].type) {
                            assert.equal(messages[i].nodeType, item.errors[i].type, "Error type should be " + item.errors[i].type);
                        }

                        if (item.errors[i].hasOwnProperty("line")) {
                            assert.equal(messages[i].line, item.errors[i].line, "Error line should be " + item.errors[i].line);
                        }

                        if (item.errors[i].hasOwnProperty("column")) {
                            assert.equal(messages[i].column, item.errors[i].column, "Error column should be " + item.errors[i].column);
                        }
                    } else {

                        // Only string or object errors are valid.
                        assert.fail(messages[i], null, "Error should be a string or object.");
                    }
                }

                if (item.hasOwnProperty("output")) {
                    var fixResult = SourceCodeFixer.applyFixes(eslint.getSourceCode(), messages);

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

            }

[nzakas]

Yes, I think so.

[gyandeeps]

@DrewML Since you have the work done (assuming since u began working on it), would like to send a PR?

[not-an-aardvark]

This is fixed as of #7550.