-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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
Chore: add a fuzzer to detect bugs in core rules #8422
Conversation
LGTM |
@not-an-aardvark, thanks for your PR! By analyzing the history of the files in this pull request, we identified @nzakas, @IanVS and @kaicataldo to be potential reviewers. |
It looks like the tests are failing because Given that this is just an internal debugging tool and isn't exposed anywhere, I think the easiest thing to do would be to skip the tests for it when the Node version is less than 6. |
Would it better as a stand-alone tool, instead of embedding it into ESLint project? |
👍 to standalone tool-- we could still use it in ESLint by consuming it as a devDependency and adding an npm script. |
Could you explain the advantage of making it a standalone tool? To me, it seems like it better to add it to eslint's repo directly because:
|
Hi! I’m the creator of eslump. I’m glad that eslump has helped you find and fix problems in ESLint. But I’m also a little worried. eslump has only ever been intended to be a CLI tool. The fact that eslump was created from the need of finding edge cases in prettier. It started out as a bare-bones little script in a branch on my fork of that repo. As I wanted more and more features, I extracted it and fleshed it out in its own repo. Then I realized that it might be useful to others, so I put it on github and made the CLI installable from npm. Initially, eslump basically just strung together shift-fuzzer-js and shift-codegen-js. Then, I realized that no random comments were generated, so I hacked that in (along with random whitespace) since comments are very difficult to get right in prettier. Then, @not-an-aardvark requested random parentheses and semicolons (lydell/eslump#1), so I hacked that in as well. I have an ambitious plan in the back of my head to create a stand-alone library that generates random JS (including flow and JSX), but I’m not sure when (or if) I’ll get around doing that. Anyway, since eslump only was intended to be a CLI tool (and its “public” API isn’t even documented), is a bit hacky and doesn’t even have tests (I’ve just gone meta and fuzz-tested it using itself basically), I feel worried about ESLint’s testing infrastructure depending on it. Therefore, I’d recommend one of these:
|
LGTM |
1 similar comment
LGTM |
@lydell Thanks for your comment -- I hadn't realized that |
FWIW, I'm fine with this being in ESLint core. |
How do we want to proceed with this? This seems like a very useful tool, and I'd love to see it implemented so we can all use it (though I don't personally feel strongly about it being in core vs a standalone tool). |
I think the main blocker at the moment is that eslump, the random code generator used by the fuzzer, doesn't support Node 4. It seems like our options are to either (a) require Node 6 for the fuzzer and skip the fuzzer tests in Node 4, or (b) make a PR/fork of eslump to support Node 4. |
@not-an-aardvark If we just made it a standalone tool that we use for development purposes, would that suffice? That way we don't necessarily need to worry about running it in Node 4, right? |
We could do that, although I think we could accomplish that even without needing to make it a standalone tool. Right now it's run as There are also a few tests for the fuzzer, which we could skip in Node 4. In my mind, the advantages of having this in the ESLint repo are that (a) it's very useful for developing ESLint, and (b) it uses internal ESLint APIs, so we would know to update it if something changes internally. The disadvantage would be that we might have to conditionally run a test file, which seems slightly messy. |
@not-an-aardvark another disadvantage is more code for users to download and for us to maintain. I still think that standalone tool that we would run nightly in jenkins would be a better choice. |
@ilyavolodin I disagree. This would not result in any more code for users to download -- it's excluded from the package when publishing. Also, if we were running it on jenkins then we would have to maintain it regardless of whether it was part of the repo or a separate tool, so I don't think putting it in the repo makes it harder for us to maintain. Since it uses internal APIs, I think making it a separate tool would actually cause more maintenance work, because we would accidentally break it when changing the APIs. The fuzzer is clearly useful for finding bugs in rules, so to me, it seems like it's definitely worth adding. Just to be clear (you might already be aware of these, but I'm restating them to make sure):
|
I would love to see it as a standalone tool so that it could be augmented
for testing plugin rules eventually. It would be awesome to be able to
offer it as a service for plugin or parser developers, and having it
bundled in ESLint as part of the public API feels like it bloats ESLint.
Just my two cents.
…On Jun 21, 2017 8:58 PM, "Teddy Katz" ***@***.***> wrote:
@ilyavolodin <https://github.com/ilyavolodin> I disagree. This would not
result in any more code for users to download -- it's excluded from the
package when publishing. Also, if we were running it on jenkins then we
would have to maintain it regardless of whether it was part of the repo or
a separate tool, so I don't think putting it in the repo makes it harder
for us to maintain.
Since it uses internal APIs, I think making it a separate tool would
actually cause more maintenance work, because we would accidentally break
it when changing the APIs. The fuzzer is clearly useful for finding bugs in
rules, so to me, it seems like it's definitely worth adding.
Just to be clear (you might already be aware of these, but I'm restating
them to make sure):
- The fuzzer is an internal-only tool -- this is just for ESLint and
doesn't add anything to our API.
- The fuzzer currently does not get run as part of the unit tests --
it's just an opt-in check with npm run fuzz.
- In its current state, the fuzzer only makes sense for testing core
ESLint rules -- it doesn't allow other rules to be tested.
- The fuzzer has been shown to be very useful for finding bugs in
rules.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#8422 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AARWesSyy52dCOFU8o4csQmPRQzgr52Sks5sGcpWgaJpZM4M2a5V>
.
|
If it helps, I guess I can add Node.js 4 support to eslump. Tell me if you want me to take a look at it! |
@not-an-aardvark Hmm.. Fair point. I forgot that we can exclude the code from the distribution. In that case, I guess I don't have a strong argument against merging this in (other then failing builds:-) |
Thanks to @not-an-aardvark, eslump 1.6.0 now has Node.js 4 support! It now also explicitly supports |
This commit adds a fuzzer to detect bugs in core rules. The fuzzer can detect two types of problems: crashes (where a rule throws an error given a certain syntax) and autofix errors (where an autofix results in a syntax error). The fuzzer works by running eslint on randomly-generated code with a random config. The code is generated with [eslump](https://github.com/lydell/eslump), and the config is generated with the existing autoconfig logic. The fuzzer can be run with `npm run fuzz`. Eventually, I think we should add the fuzzer to the normal `npm test` build.
LGTM |
Thanks @lydell! ESLint team: I've updated this PR to work correctly with the ESLint 4.0 refactor, and I've upgraded eslump so it supports Node 4. This should now be ready to be reviewed. |
What is the purpose of this pull request? (put an "X" next to item)
[x] Other, please explain:
What changes did you make? (Give an overview)
This commit adds a fuzzer to detect bugs in core rules. The fuzzer can detect two types of problems: crashes (where a rule throws an error given a certain syntax) and autofix errors (where an autofix results in a syntax error).
The fuzzer works by running eslint on randomly-generated code with a random config. The code is generated with eslump, and the config is generated with the existing autoconfig logic.
I've been using and developing this fuzzer locally for the past few weeks. It has been immensely helpful for finding bugs in rules. (It discovered the bugs fixed by #8245, #8246, #8247, #8327, #8328, #8330, #8331, #8332, #8335, #8337, #8338, #8339, #8340, #8358, #8359, #8391, #8394, #8412, #8806, #8807, and #8808.)
The fuzzer can be run with
npm run fuzz
. Eventually, I think we should add the fuzzer to the normalnpm test
build. Right now it's still reporting a few errors sometimes, which we should probably investigate, but we shouldn't let that break the regular build.Is there anything you'd like reviewers to focus on?
tools/
. Let me know if there's a better place to put it.debug/
folder, which is in gitignore. Let me know if there's a better place to put it.