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

Fuzz test location-related parser options #14201

Merged
merged 34 commits into from Jan 29, 2022

Conversation

tolmasky
Copy link
Contributor

@tolmasky tolmasky commented Jan 25, 2022

Q                       A
Fixed Issues?
Patch: Bug Fix? Yes
Major: Breaking Change? No
Minor: New Feature? No
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

This PR completes the work started in #14130 by adding the necessary foundations to do fuzz testing in our tests, and implements the first instance of such fuzz testing with startLine and startColumn. Basically, it takes all the existing tests, and runs them all with several different combinations of startLine and startColumn (including having them absent in the options object, etc.). This increases the number of total tests we run from 22566 to 88344.

  1. Despite effectively quadrupling the number of tests, they actually complete in the same amount of time or faster. For example, the Node.js 14 tests complete in 109s vs. 119s in CI. This is due to a number of performance improvements in the way we compare ASTs, etc. There's way more performance stuff I'd like to do, but I tried to limit this PR to as small of a footprint as possible. By changing getFixtures and the actual test format, it should be possible to dramatically improve the performance of tests further (especially when running only a subset with TEST_GREP, which currently still pays a huge price for all the tests it doesn't run).

  2. The sole exception to this is node 8, where tests were taking an extremely long time after this change. After spending way too much time trying to figure this out, I discovered that Jest 23 on node 8 just has a threshold number of tests where afterward it simply takes forever (40 min+) no matter what, even if the tests are completely empty. As such, I've simply turned off fuzz testing in Node 8, given that support for node 8 is going to be dropped in Babel 8 anyways. Interestingly, Node 6 does not have this problem.

  3. You can turn fuzz testing off by setting the FUZZ environment variable to false: FUZZ=false. There should really be no reason to though, as the tests take the same amount of time to complete.

  4. This PR also fixes showing code frames, which was broken I believe in this commit due to calling error.toString(), which does not include the stack. The behavior is actually improved, as you'll now see code frames even in the recovered errors:

Screen Shot 2022-01-25 at 4 17 31 PM

  1. I renamed "runFixtureTests.js" to "run-fixture-tests.js" to avoid case sensitive filesystem issues. Happy to change this back if it is not desired.

  2. There's a Difference class that calculates differences between ASTs in a way that's easy to manipulate after-the-fact:

    const difference = new Difference(adjust, expect, actual);
    
    // difference.path === [`ast`, `body`, 0, `expression`, `value`]
    // difference.reason === { type: "value", expected: 5, actual: 6 }
    // difference.message = "5 != 6 in .ast.body[0].expression.value"

    As you can see, the error message now uses a "JavaScript compliant" accessor path (".ast.body[0].expression.value"), instead of the "filesystem-style" path it used to present (something like "/ast/body/0/expression/value"). This makes it easy to copy-paste the path into a REPL if you are trying to debug the problem. It also fixes a bug with the regex replacement stuff that was done with string manipulation in the old system. It uses isIdentifierName internally, so it should always generate a JavaScript-friendly path.

    You'll notice there is an adjust function that is passed to Difference. This is one of the things that allows us to make fuzz testing quick. Instead of either generating a different AST for each expected parse with different options, or even simply "modifying" the expected AST for each fuzz option, the adjust function travels along the comparison calls (much like the serialize function in JSON.stringify), and is given a chance to modify the value that is compared. So, for example, if the fuzzed options were "startLine = 10, startColumn = 20", the generated adjust function does something along these lines:

    function adjust(value, key, original) {
      if (key === "line") {
         return value - 1 + startLine; // 1 -> 20, 2 -> 21, etc.
      }
      // Only adjust the column for "line 1"
      if (key === "column" && original.line === 1) {
         return value + startColumn;
      }
      return value;
    }

    It's a bit more complicated since the function is auto-generated of course, and because it also has to account for if the original test itself included a startLine, but this is the basic idea. You can see that this could fairly trivially be expanded to automatically testing errorRecovery being true and false (explained in more detail below).

  3. FixtureError has been expanded to give some additional semantic meaning. There's now FixtureError.DifferentAST, FixtureError.UnexpectedError, FixtureError.UnexpectedRecovery, etc. This generally makes it easier to keep track of what's going on and generate nice error messages, instead of repeatedly throwing and catching and modifying message strings. The correct FixtureError is obtained by passing the Difference:

    const error = FixtureError.fromDifference(difference, actual);
  4. There is no more need for runFixtureTestsWithoutExactASTMatch, as the onlyCompareErrors option has been pushed up to the runFixtureTests API, so it can simply be called with true as the last argument to get this behavior.

  5. As I mentioned above, I specifically stayed away from changing file formats, but I think one of the next steps is to properly serialize errors. We currently don't actually really test whether startLine and startColumn affect errors, since we only serialize their error messages (and in subtly different ways depending on whether the error was recovered from or not). We do however test this when the line and column number happen to be in the error message itself.

With this foundation in place, my plan is to slowly incorporate more of the options into our fuzz testing, to get more bang for our buck from our existing tests, and to additionally discover more edge cases. For example, the one I'd like to do next is to automatically try each test with errorRecovery set to both true and false (and missing). Given that tests are already generated with errorRecovery set to true, when we run it with false, we simply have to test that the error thrown is equal to the first error in the errors array. This fuzz test would have for example caught this bug: #14146

Generally speaking, the "ideal" would be to eventually handle all options through this system, such that the total number of effective tests is [base_tests] * 2 ^ (# of boolean options) * (# of multi-state-options ...). Through performance work on the tests like the above, this should have negligible effects on our runtime, but potentially offer a great benefit in discovering bugs when running babel with complicated combinations of options without requiring a ton of manual labor to keep adding such tests.

…here. Also improve the performance of tests so that the fact that we have ~88,000 tests now doesn't really cause the tests to run any slower.

Reviewed by @tolmasky.
…8 takes forever after a certain number of tests, even if those tests are empty.

Reviewed by @tolmasky.
@nicolo-ribaudo nicolo-ribaudo added the PR: Internal 🏠 A type of pull request used for our changelog categories label Jan 25, 2022
@tolmasky
Copy link
Contributor Author

I forgot to include one minor point: I made a small format change in that the 2 (!) tests that use the babel serialization key to encode either RegExps or bigints in their AST JSON files now have the file extension ".extended.json" instead of ".json". This just helps a ton by allowing me to entirely avoid passing a reviver function to JSON.parse for the thousands of other tests that don't do any special encoding in their JSON serialization. The test generator is smart and will automatically give the file the correct extensions, so it shouldn't add any work for anyone.

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks really good!

packages/babel-parser/test/helpers/difference.js Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
packages/babel-parser/test/helpers/difference.js Outdated Show resolved Hide resolved
packages/babel-parser/test/helpers/fixture-error.js Outdated Show resolved Hide resolved
packages/babel-parser/test/helpers/to-fuzzed-options.js Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@tolmasky
Copy link
Contributor Author

I think that addresses all the comments, but please let me know if I missed any.

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@nicolo-ribaudo nicolo-ribaudo changed the title Fuzz testing Fuzz test location-related parser options Jan 26, 2022
Copy link
Contributor

@JLHwung JLHwung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do a more thorough review later.

Reviewed by @tolmasky.
Copy link
Contributor

@JLHwung JLHwung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work!

packages/babel-parser/test/helpers/run-fixture-tests.js Outdated Show resolved Hide resolved
… error message when there is no cause.

Reviewed by @tolmasky.
@tolmasky
Copy link
Contributor Author

Thanks!

Copy link
Member

@existentialism existentialism left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome!

@nicolo-ribaudo nicolo-ribaudo merged commit 6b427ce into babel:main Jan 29, 2022
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label May 1, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: tests outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Internal 🏠 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants