New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

messageId messages with placeholders fail during tests. #9890

Closed
betaorbust opened this Issue Jan 25, 2018 · 30 comments

Comments

Projects
None yet
4 participants
@betaorbust
Contributor

betaorbust commented Jan 25, 2018

Tell us about your environment

  • ESLint Version: 4.15+
  • Node Version: 8.9.3
  • npm Version: 5.5.1

What parser (default, Babel-ESLint, etc.) are you using?
default

Please show your full configuration:
N/A

What did you do? Please include the actual source code causing the issue, as well as the command that you used to run ESLint.
Write a test where the message is a placeholder and the message is referred to via the new meta.messageId. Run the test runner against it with a messageId to match against.

lib/rules/no-foo.js
module.exports = {
    meta: {
        messages: {
            avoidName: "Avoid using variables named '{{ name }}'"
        }
    },
    create(context) {
        return {
            Identifier(node) {
                if (node.name === 'foo') {
                    context.report({
                        node,
                        messageId: 'avoidName',
                        data: {
                            name: 'foo'
                        }
                    });
                }
            }
        };
    }
};
test/rules/no-foo.js
'use strict';
var rule = require('../../../lib/rules/no-foo');
var RuleTester = require('eslint').RuleTester;

var ruleTester = new RuleTester();
ruleTester.run('no-foo', rule, {
    valid: ['bar', 'baz'],
    invalid: [
        {
            code: 'foo',
            errors: [
                {
                    messageId: 'avoidName'
                }
            ]
        }
    ]
});
npm run test

What did you expect to happen?
Tests pass.

What actually happened? Please include the actual, raw output from ESLint.

  1 failing

  1) no-foo invalid foo:

      AssertionError [ERR_ASSERTION]: 'Avoid using variables named \'foo\'' === 'Avoid using variables named \'{{ name }}\''
      + expected - actual

      -Avoid using variables named 'foo'
      +Avoid using variables named '{{ name }}'

      at assertMessageMatches (node_modules\eslint\lib\testers\rule-tester.js:442:24)
      at testInvalidTemplate (node_modules\eslint\lib\testers\rule-tester.js:517:29)
      at Context.RuleTester.it (node_modules\eslint\lib\testers\rule-tester.js:581:25)

So it looks like ruleTester is comparing the value of messageId's pre-formatted template to the post-formatted runtime error.

It seems like the ideal behavior is:

  1. If your test needs to get the post-formatted runtime report message, use message in the ruleTester.
  2. If your test wants to know that a specific rule ID was used to generate the error message, use mesageId in the ruleTester.

My guess is that most use cases fall under the second category, but that the first category is worth supporting as well to give access to the direct output during testing if needed.

@platinumazure

This comment has been minimized.

Member

platinumazure commented Jan 25, 2018

Thanks for the bug report! This definitely needs another look, for sure.

@j-f1

This comment has been minimized.

Contributor

j-f1 commented Jan 25, 2018

I didn’t think of that use case when making this API. See #9892 for how I intended the tests to be written. However, now that I see this, I think it would be a good idea to allow people to only specify some (or none) of the data keys in a test.

@betaorbust

This comment has been minimized.

Contributor

betaorbust commented Jan 25, 2018

Looking through the rule-tester code, it looks like the anticipated use case is to store the data for formatting the error in the test file itself.
https://github.com/eslint/eslint/blob/master/lib/testers/rule-tester.js#L511-L517

This doesn't really help isolate the string information, which would be ideal for maintainability.

Previously, I've been using the following pattern to keep a single source of truth -- but also doesn't work out of the box for formatted errors.

// rules/my-rule.js
module.exports = { \* actual rule code \*};
module.exports.errorStrings = { \* store error strings on the exported object *\ }


// test/rule.js
const rule = require('../rules/my-rule');
const errorStrings = rule.errorStrings;
const RuleTester  = require('eslint').RuleTester;
const ruleTester = new RuleTester();
ruleTester.run('my-rule', rule, {
    valid: [/* valid case... */]
    invalid: [{
        code: 'test case',
		errors: [{
			message: errorStrings.myError,
			type: 'Literal'
		}]
	}]
});

Ideally, if we're using IDs for messages, we would just be able to refer and validate against them. It would hopefully be a win for both maintainability as well as localization etc.

@betaorbust

This comment has been minimized.

Contributor

betaorbust commented Jan 25, 2018

@j-f1 Makes total sense.
Perhaps there's something we can do in the ruleTester that would match the id directly instead of formatting the underlying message?

Like

ruleTester.run('my-rule', rule, {
    errors: [{
        rawMessageId: 'myErrorMessageId',
        type: 'Literal'
    }]
})

Hopefully we could come up with something better than rawMessageId 😉

@platinumazure

This comment has been minimized.

Member

platinumazure commented Jan 27, 2018

Here's what I think we should consider (@j-f1 I assume most of this is already happening, but let me know if I'm missing something):

  • If user tests for message and messageId, throw an error
  • If a user tests for a message and data, I think we should consider throwing an error
    • Presumably this would be a breaking change if we implement this
  • If user tests for a messageId and data but the rule uses a message (possibly including data), fail the test
  • If user tests for a message but the rule has messageId and data, then RuleTester should hydrate the base message with the data and compare the resulting message strings
    • Optional/Future enhancement: Maybe RuleTester could take an option to fail in this case and require users to test for messageId and data, but that shouldn't be default behavior for sure
  • If user tests for a messageId and data and the rule passes messageId and data, assert that the messageIds match and that any passed in data keys match, but allow any missing keys.
    • If user does not test for data, just messageId, allow that as well.
    • Optional/Future enhancement: Maybe RuleTester could take an option to fail in this case and require that all of the data keys used by the rule are present and have equal value in the test

How does that sound? Have I missed anything? How far off are we right now?

@betaorbust

This comment has been minimized.

Contributor

betaorbust commented Jan 27, 2018

  • If user tests for a messageId and data and the rule passes messageId and data, assert that the messageIds match and that any passed in data keys match, but allow any missing keys.
    If user does not test for data, just messageId, allow that as well.
    • Optional/Future enhancement: Maybe RuleTester could take an option to fail in this case and require that all of the data keys used by the rule are present and have equal value in the test

I think this is where we might run afoul of the current implementation; right now, when messageId is provided, we fetch the template string from the rule, hydrate it with the provided test data, and then compare the resulting interpolated test message with the message coming out of the rule.
https://github.com/eslint/eslint/blob/master/lib/testers/rule-tester.js#L511-L517

We could change this behavior or we could introduce a different property (like the rawMessageId I proposed above) which would take on this functionality. I'm not a huge fan of the second solution, although I have it running in a local branch -- I think for the long-term developer experience win of making messageId check against the messageId directly (and data check against data in this case) is worth the minor change in underlying behavior.

@betaorbust

This comment has been minimized.

Contributor

betaorbust commented Jan 27, 2018

Reworded a bit to reflect the data we have on hand at runtime and to make it easy to turn into tests. Tell me what you think of the following:

  1. Nominal cases:
    1. should assert match if message provided in both test and result.
    2. should assert match between messageId if provided in both test and result and test data is undefined.
    3. should assert match between messageId and data if provided in both test and result.
      • Note: If this isn't a deep compare between test.data and result.data, then there is no way to test for "Was called without data" due to spec 1.ii.
  2. Misconfiguration cases:
    1. should throw if user tests for both message and messageId
    2. should throw if user tests for messageId but the rule doesn't use the messageId meta syntax.
    3. If user tests for a message but the rule has messageId and data, then RuleTester should hydrate the base message with the data and compare the resulting message strings
      • Note: Because the results are already hydrated, this is covered by the simpler/existing rule.message === error.message check.
      • Optional/Future enhancement: Maybe RuleTester could take an option to fail in this case and require users to test for messageId and data, but that shouldn't be default behavior for sure.
    4. should throw if user tests for message as well as data.
      • Note: Not a specific blocker/bug but might consider doing this in the future to limit the API space and clear up the blessed path.
@j-f1

This comment has been minimized.

Contributor

j-f1 commented Jan 27, 2018

LGTM 👍

@not-an-aardvark

This comment has been minimized.

Member

not-an-aardvark commented Jan 28, 2018

Does RuleTester currently have a way of accessing the data provided by rules? I was under the impression that it does not have access, although my memory might be out of date.

Assuming data is not currently exposed, I'm not sure how I feel about exposing it as part of results objects (which would be provided to RuleTester, formatters, and public API clients) -- it's not clear to me that it's worth increasing the API surface. Could you clarify the use case for matching against data?

@betaorbust

This comment has been minimized.

Contributor

betaorbust commented Jan 28, 2018

It does not currently have access to the data provided by rules.
The original intent of messageId (I believe) was to reduce the duplication of test code and make maintenance easier. It also has the follow-on impact, paired with data, of laying the groundwork for localization.
If we have messageId, but not the data, we can go one of two ways:

  • Only check that the messageIds match, which limits the usefulness of the check in situations where you want to be more precise.
  • Require providing data in each test case, which means over-duplication/higher maintenance when a tester only wants to validate the messageId used.

To be clear, I’m almost never for expanding public interfaces, but I think the relatively smooth path to messageId/data, the fact that messageId paired with data is a complete source of truth, and the large potential upsides for both maintenance as well as localization makes it worth it.

@platinumazure

This comment has been minimized.

Member

platinumazure commented Jan 28, 2018

I'd be interested to see if we could explore separating out the message hydration logic, so that it would be possible to call the rule and get messageId and data (or, for that matter, message and data with no interpolation applied), and then RuleTester could choose to either invoke the message hydration logic, or just compare messageId and data. Specifically, I'm interested in doing this so that only RuleTester could potentially do this internally, and formatters and public API clients would never see this.

@not-an-aardvark Would my proposal above address your concerns about expanding the API surface? (Of course, the added complexity might not be worth it anyway, but I wanted to get your thoughts about expanding the surface only for RuleTester.)

@platinumazure

This comment has been minimized.

Member

platinumazure commented Jan 28, 2018

Relabeling as enhancement rather than bug, since it seems like this is going slightly beyond a bugfix. The TSC will probably need to review/approve whatever changes we decide to propose here.

@betaorbust No worries about making a PR, but just note that we might go back and forth on a few proposals here until we get something that the team agrees is the best way forward. I apologize for any confusion that resulted from the initial labeling as a bug.

@betaorbust

This comment has been minimized.

Contributor

betaorbust commented Jan 28, 2018

Totally happy for this to be addressed at a larger and more orchestrated level.
My main drive with this is allowing rule testers to

  • check that a messageId was used, independent of data used to format it.
  • check the specific data used to format a messageId (or if any data was used at all)

The large number of rule tests (in the main eslint codebase and in 3rd-part plugins) that are partial regex matches against the message string tells me that I’m not the only one wanting to do this.

@betaorbust

This comment has been minimized.

Contributor

betaorbust commented Jan 28, 2018

While the data portion of this is being worked out, I’d like to propose fixing the unexpected behavior of checking messageId today.
Right now, it checks the rules string associated with the messageId and strictly compares it against the resulting message. I think that functionality is unexpected, as the other things in rule tester are identity checks against the values they supply.
I propose a change to make the messageId check directly against the messageId used at runtime.
Thoughts @platinumazure @not-an-aardvark and @j-f1?

betaorbust added a commit to betaorbust/eslint that referenced this issue Jan 28, 2018

Fix: Make rule-tester strictly check messageId. (ref eslint#9890)
This change makes rule-tester check messageId directly by value provided instead
of the current behavior, which is to check the messageId's message value against
the message returned from the rule at runtime.
@betaorbust

This comment has been minimized.

Contributor

betaorbust commented Jan 28, 2018

#9908 For proposed changes to messageId comparison to bring RuleTester's direct comparison treatment of other error properties to messageId.
It addresses the original intent of this issue, and leaves the data comparison discussion to be hammered out in time.

@not-an-aardvark

This comment has been minimized.

Member

not-an-aardvark commented Jan 29, 2018

Would my proposal above address your concerns about expanding the API surface? (Of course, the added complexity might not be worth it anyway, but I wanted to get your thoughts about expanding the surface only for RuleTester.)

That would address the concerns, although ideally it would be nice if the functionality of RuleTester were implementable using the public API (so that if someone didn't like RuleTester they could create their own customized version with similar functionality).


Personally, I think the following behavior would make the most sense, and wouldn't require data to be exposed:

  • If the test provides messageId, ensure that it matches the messageId from the reported problem.
  • If the test provides message, ensure that it matches the message from the reported problem
  • If the test provides messageId and data, hydrate the message for the provided messageId with the provided data and compare it with the message from the reported problem

All of these three assertions would be independent, and more than one could be checked for a given test case.

Additionally, all of the following cases would be invalid:

  • Providing data without messageId
  • Providing a messageId which does not exist
  • (Maybe) Providing messageId and message simultaneously

I think this is similar to the existing behavior, except that (a) we would match against the messageId of the reported problem regardless of whether data is provided, and (b) we wouldn't match against message when data is omitted.

@betaorbust

This comment has been minimized.

Contributor

betaorbust commented Jan 29, 2018

  • If the test provides messageId and data, hydrate the message for the provided messageId with the provided data and compare it with the message from the reported problem.

This seems like it complicates the expected use of the RuleTester API as the rest of the attributes are independent and direct-comparison (and getting stricter via #9417 in 5.0) but it does free up the solo messageId case, which was my initial bug report, so 👍

I'm very familiar with this code at this point, so just give me the green light when you guys finalize the proposal and I can drop a PR.

@not-an-aardvark

This comment has been minimized.

Member

not-an-aardvark commented Jan 29, 2018

This seems like it complicates the expected use of the RuleTester API as the rest of the attributes are independent and direct-comparison

That's a fair point. I think data is a bit different from other properties in that the "important" keys are dependent on the message/messageId anyway. For example, some rules provide an AST node as the data property and rely on ESLint to grab the appropriate properties from it:

context.report({
node,
message: "'{{name}}' is never reassigned. Use 'const' instead.",
data: node,
fix: shouldFix ? fixer => fixer.replaceText(sourceCode.getFirstToken(varDeclParent), "const") : null
});

With code like that, it probably wouldn't make sense to match against every property of node, since there are a lot of unrelated properties and the data is functionally equivalent to { name: node.name }. So either way, I think there is a coupling between message/messageId and data that doesn't exist between other properties, so I'm fine with having the corresponding RuleTester options coupled.

@betaorbust

This comment has been minimized.

Contributor

betaorbust commented Jan 29, 2018

Yeah, looks like there are a lot of places that do that. It kind of seems like an antipattern to not be explicit about what’s being used, but the rework effort would be substantial.
Makes total sense.

@not-an-aardvark

This comment has been minimized.

Member

not-an-aardvark commented Feb 1, 2018

@platinumazure Do you have any thoughts on the proposal in #9890 (comment)?

  • Any disagreements?
  • Should we put this on the TSC agenda as an enhancement, or do you think it would be considered a bugfix (since it no longer adds anything to the report objects)?
@platinumazure

This comment has been minimized.

Member

platinumazure commented Feb 1, 2018

@not-an-aardvark I'm okay with that proposal. This means it is impossible for now to use messageId with partial data, but I think that's acceptable for now since we could theoretically add support for partial data in a backward-compatible way later on. Thanks for following up!

@betaorbust

This comment has been minimized.

Contributor

betaorbust commented Feb 1, 2018

So, everybody good if I put a PR in that satisfies #9890 (comment)?

@platinumazure

This comment has been minimized.

Member

platinumazure commented Feb 2, 2018

@betaorbust Yes, I think that's a good approach.

Does #9908 satisfy the requirements described in the comment? If not, please close (or, if you just need to make a few modifications in the branch, please edit the PR title to say "WIP" so we don't merge it accidentally). Thanks!

@betaorbust betaorbust changed the title from messageId messages with placeholders fail during tests. to WIP: messageId messages with placeholders fail during tests. Feb 25, 2018

@betaorbust betaorbust changed the title from WIP: messageId messages with placeholders fail during tests. to messageId messages with placeholders fail during tests. Feb 25, 2018

betaorbust added a commit to betaorbust/eslint that referenced this issue Feb 25, 2018

Fix: Make rule-tester strictly check messageId. (ref eslint#9890)
This change makes rule-tester check messageId directly by value provided instead
of the current behavior, which is to check the messageId's message value against
the message returned from the rule at runtime.

betaorbust added a commit to betaorbust/eslint that referenced this issue Feb 25, 2018

Fix: Make rule-tester strictly check messageId. (ref eslint#9890)
This change makes rule-tester check messageId directly by value provided instead
of the current behavior, which is to check the messageId's message value against
the message returned from the rule at runtime.

betaorbust added a commit to betaorbust/eslint that referenced this issue Feb 26, 2018

Fix: Make rule-tester strictly check messageId. (ref eslint#9890)
This change makes rule-tester check messageId directly by value provided instead
of the current behavior, which is to check the messageId's message value against
the message returned from the rule at runtime.
@not-an-aardvark

This comment has been minimized.

Member

not-an-aardvark commented Mar 16, 2018

@platinumazure This issue is marked as core/enhancement/evaluating, which would normally imply that we need to get TSC consensus on it before accepting a change. Are the labels still accurate, or do you think the modified proposal would be considered a bugfix?

@platinumazure

This comment has been minimized.

Member

platinumazure commented Mar 16, 2018

@not-an-aardvark I think this could be considered a bugfix since we're not really trying to add new functionality, but rather stabilize this API for tests in a way that meshes with the original vision. I'm okay with relabeling as bug.

@betaorbust

This comment has been minimized.

Contributor

betaorbust commented Mar 16, 2018

Awesome. As a bug fix, is it ready to go in or do you need anything else from me?

@platinumazure

This comment has been minimized.

Member

platinumazure commented Mar 16, 2018

@betaorbust I think it just needs review at this point. Just to be absolutely sure, is #9908 the PR that goes with this issue, that we should review? Thanks!

@betaorbust

This comment has been minimized.

Contributor

betaorbust commented Mar 16, 2018

Yup. I closed the breaking change/enhancement PR and updated #9908 as described in #9890 (comment), so it should be good to go.

platinumazure added a commit that referenced this issue Mar 22, 2018

Fix: Make rule-tester strictly check messageId. (ref #9890) (#9908)
This change makes rule-tester check messageId directly by value provided instead
of the current behavior, which is to check the messageId's message value against
the message returned from the rule at runtime.
@platinumazure

This comment has been minimized.

Member

platinumazure commented Mar 27, 2018

@betaorbust After merging #9908, is there anything else that we still want to try to address with this issue? Or can we close the issue? Thanks!

@betaorbust

This comment has been minimized.

Contributor

betaorbust commented Mar 27, 2018

@platinumazure Yup. My preference would have been to make it an even more restrictive API, but #9890 (comment) makes sense, so I think we can close this out 👍

@betaorbust betaorbust closed this Mar 27, 2018

@eslint eslint bot locked and limited conversation to collaborators Sep 24, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.