Skip to content
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

Merged
merged 4 commits into from Jul 9, 2017
Merged

Chore: add a fuzzer to detect bugs in core rules #8422

merged 4 commits into from Jul 9, 2017

Conversation

not-an-aardvark
Copy link
Member

@not-an-aardvark not-an-aardvark commented Apr 7, 2017

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 normal npm 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?

  • I wasn't sure where to put the fuzzer, so I created a new top-level directory called tools/. Let me know if there's a better place to put it.
  • The fuzzer outputs debugging info when it fails. Right now, the info is output to a top-level debug/ folder, which is in gitignore. Let me know if there's a better place to put it.

@not-an-aardvark not-an-aardvark added accepted There is consensus among the team that this change meets the criteria for inclusion chore This change is not user-facing labels Apr 7, 2017
@eslintbot
Copy link

LGTM

@mention-bot
Copy link

@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.

@not-an-aardvark not-an-aardvark removed the accepted There is consensus among the team that this change meets the criteria for inclusion label Apr 7, 2017
@not-an-aardvark
Copy link
Member Author

It looks like the tests are failing because eslump only supports Node 6+.

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.

@ilyavolodin
Copy link
Member

Would it better as a stand-alone tool, instead of embedding it into ESLint project?

@platinumazure
Copy link
Member

👍 to standalone tool-- we could still use it in ESLint by consuming it as a devDependency and adding an npm script.

@not-an-aardvark
Copy link
Member Author

not-an-aardvark commented Apr 7, 2017

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:

  • I think it should be easy to run it while developing eslint (and eventually it should be run as part of the tests). It has been very useful for finding other eslint bugs, as discussed above.
  • It uses a few private ESLint APIs
  • If its main user is the eslint project, seems like it would be useful for the eslint team to be able to make modifications to it.

@lydell
Copy link

lydell commented Apr 8, 2017

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 index.js exposes generateRandomJS is mostly by accident, and has stuck around since then.

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:

  • Pin eslump to a fixed version in package.json.
  • Copy codegen.js from eslump to somewhere appropriate.

@eslintbot
Copy link

LGTM

1 similar comment
@eslintbot
Copy link

LGTM

@not-an-aardvark
Copy link
Member Author

@lydell Thanks for your comment -- I hadn't realized that generateRandomJS wasn't supposed to be exposed. For now, I've pinned the eslump version in this PR.

@kaicataldo
Copy link
Member

FWIW, I'm fine with this being in ESLint core.

@kaicataldo
Copy link
Member

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).

@not-an-aardvark
Copy link
Member Author

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.

@kaicataldo
Copy link
Member

@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?

@not-an-aardvark
Copy link
Member Author

not-an-aardvark commented Jun 21, 2017

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 npm run fuzz, so we could just print an error message and exit if someone runs npm run fuzz on Node 4.

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.

@ilyavolodin
Copy link
Member

@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.

@not-an-aardvark
Copy link
Member Author

@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.

@platinumazure
Copy link
Member

platinumazure commented Jun 22, 2017 via email

@lydell
Copy link

lydell commented Jun 22, 2017

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!

@ilyavolodin
Copy link
Member

@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:-)

@lydell
Copy link

lydell commented Jun 26, 2017

Thanks to @not-an-aardvark, eslump 1.6.0 now has Node.js 4 support! It now also explicitly supports var generateRanomJS = require('eslump').generateRanomJS. (I've moved all the CLI stuff out of index.js, so there shouldn't be any overhead from loading unnecessary CLI modules.) The disclaimer about no tests is still there, so it's up to you to choose if you want to pin the version number or not. But nothing's changed in eslump the past 2-3 months, and there are no changes planned either.

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.
@eslintbot
Copy link

LGTM

@not-an-aardvark
Copy link
Member Author

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.

@ilyavolodin ilyavolodin merged commit 933a9cf into master Jul 9, 2017
@not-an-aardvark not-an-aardvark deleted the fuzz branch July 9, 2017 17:55
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion chore This change is not user-facing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants