-
Notifications
You must be signed in to change notification settings - Fork 22
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
Allow tests with identical names in different modules in no-identical-names
rule
#131
Allow tests with identical names in different modules in no-identical-names
rule
#131
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for putting this together! Left one question, let me know what you think.
test('it1', function (assert) {}); | ||
}); | ||
}); | ||
test('it1', function (assert) {}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this second test, outside all modules, be flagged? I feel like it should.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it should be flagged, so I marked this PR as a draft since I am going to need to do some refactoring / add a stack to handle this case.
test("it1", function() {});
module("module1", function() {});
test("it1", function() {}); // This should be a violation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Let me know when it's ready for review again or if you need any help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@platinumazure I'm having some second thoughts about this. Technically, the second test
is part of a separate module, so we should allow it. If we want to disallow top-level tests from having the same title even if they're in different modules, that would require a special case, and I'll likely want to handle that in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, the second
test
is part of a separate module, so we should allow it. If we want to disallow top-level tests from having the same title even if they're in different modules, that would require a special case, and I'll likely want to handle that in a separate PR.
Would you mind elaborating on this? What do you mean the tests are in separate modules?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad, I was wrong about that, the tests from my last example are in the same module, thus being considered a violation. To clarify, here are two test cases:
Invalid:
test("it1", function() {});
module("module1", function() {});
test("it1", function() {}); // Violation
Valid:
test("it1", function() {});
module("module1");
test("it1", function() {}); // This is in module1, so not a violation
I have updated the implementation to reflect this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @bmish for clarifying. I'll give this another review this weekend.
dd0a8e9
to
81c26ac
Compare
@platinumazure ready for review, fixes the ticket linked, and significantly cleans up / simplifies the rule logic which had become messy over time. |
81c26ac
to
6f389f9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thanks for contributing!
@platinumazure can you publish a patch release with this? |
@bmish I published a minor release just now! |
Significantly refactors/simplifies logic.
Fixes #130.