feat!: detect duplicate test cases #17955
Conversation
eslint-github-bot
bot
added
breaking
✅ Deploy Preview for docs-eslint canceled.
|
|
We are waiting on #17654 for this, correct? |
|
No I think we can treat them as independent. |
* main: chore: Split Docs CI from core CI (eslint#17897) fix: Ensure config keys are printed for config errors (eslint#17980) chore: delete relative-module-resolver.js (eslint#17981) docs: migration guide entry for `no-inner-declarations` (eslint#17977) docs: Update README feat: maintain latest ecma version in ESLint (eslint#17958) feat!: no-inner-declaration new default behaviour and option (eslint#17885) feat: add `no-useless-assignment` rule (eslint#17625) fix: `no-misleading-character-class` edge cases with granular errors (eslint#17970) feat: `no-misleading-character-class` granular errors (eslint#17515) docs: fix number of code-path events on custom rules page (eslint#17969) docs: reorder entries in v9 migration guide (eslint#17967) fix!: handle `--output-file` for empty output when saving to disk (eslint#17957)
* main: docs: Update note about ECMAScript support (eslint#17991) chore: update `markdownlint` to `v0.33.0` (eslint#17995) docs: Update release blog post template (eslint#17994) docs: Add sections on non-npm plugin configuration (eslint#17984) 9.0.0-alpha.1 Build: changelog update for 9.0.0-alpha.1 chore: package.json update for @eslint/js release
|
This is ready for review. |
lib/rule-tester/rule-tester.js
Outdated
| * @private | ||
| */ | ||
| function checkDuplicateTestCase(item, seenTestCases) { | ||
| if ((Array.isArray(item.options) && item.options.some(i => typeof i === "object")) || item.settings || item.languageOptions?.parser || item.languageOptions?.parserOptions || item.plugins) { |
There was a problem hiding this comment.
there is a room to improve it, but I think it's fine to just check these properties 👍 - as it's described in the rfc.
There was a problem hiding this comment.
It's a start.
However, I would love it if I could replace this bail-out logic with a simple function call to see if a given object is serializable. Surprisingly, I haven't found a third-party library to do this. Another option is to try to piece together such a function based on StackOverflow snippets (https://stackoverflow.com/questions/30579940/reliable-way-to-check-if-objects-is-serializable-in-) etc and add unit tests for it. Thoughts?
It might be best to solve this the right way now (in a way that avoids a lot of false negatives) since changing this later could be a breaking change.
There was a problem hiding this comment.
IMHO, it's not worth adding a new dependency: rule tester is shipped with eslint, but most eslint users don't really use it. A simple implementation to cover 90%+ cases is good enough for me. 😄
There was a problem hiding this comment.
I went ahead and added a isSerializable() function which uses only the tiny lodash.isplainobject as a dependency. While the previous approach could have been sufficient as you mentioned, this new approach is a lot more robust (no false negatives) and the code is cleaner.
|
@christian-bromann looks like our browser tests are failing again, can you take a look? |
|
@nzakas on it |
@nzakas @bmish I've discovered a performance regression with the recent release which forced me to find the culprit which I did successfully. The browser tests should run much faster now and should be more reliable too. Please re-trigger the build and the fix should be picked up automatically. |
mdjermanovic
added
the
accepted
It works now, thanks! |
eslint-github-bot
bot
added
the
archived due to age
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[x] Add something to the core
[ ] Other, please explain:
What changes did you make? (Give an overview)
Fixes #15104, alongside:
Link to this feature in the RFC:
Inspired by:
Is there anything you'd like reviewers to focus on?
Are we okay with adding this
isSerializable()function? Surprisingly, I couldn't find any third-party library that offers it. I had an alternative approach visible in b400d1e that simply checked for the presence of any properties that might not be serializable, but this could have false negatives. See discussion in #17955 (comment).TODO: