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
Conversation
…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.
Reviewed by @tolmasky.
Reviewed by @tolmasky.
…ert them. Reviewed by @tolmasky.
Reviewed by @tolmasky.
Reviewed by @tolmasky.
Reviewed by @tolmasky.
Reviewed by @tolmasky.
Reviewed by @tolmasky.
Reviewed by @tolmasky.
Reviewed by @tolmasky.
Reviewed by @tolmasky.
Reviewed by @tolmasky.
Reviewed by @tolmasky.
…e original options. Reviewed by @tolmasky.
Reviewed by @tolmasky.
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 |
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, this looks really good!
I think that addresses all the comments, but please let me know if I missed any. |
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.
Thank you!
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.
Will do a more thorough review later.
…ocs comments. Reviewed by @tolmasky.
Reviewed by @tolmasky.
e2ffbd7
to
b1118e4
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.
Awesome work!
… error message when there is no cause. Reviewed by @tolmasky.
Thanks! |
Reviewed by @tolmasky.
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.
This is awesome!
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
andstartColumn
. Basically, it takes all the existing tests, and runs them all with several different combinations ofstartLine
andstartColumn
(including having them absent in theoptions
object, etc.). This increases the number of total tests we run from22566
to88344
.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).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.
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.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: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.
There's a
Difference
class that calculates differences between ASTs in a way that's easy to manipulate after-the-fact: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 toDifference
. 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, theadjust
function travels along the comparison calls (much like theserialize
function inJSON.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 generatedadjust
function does something along these lines: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
beingtrue
andfalse
(explained in more detail below).FixtureError
has been expanded to give some additional semantic meaning. There's nowFixtureError.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 correctFixtureError
is obtained by passing theDifference
:There is no more need for
runFixtureTestsWithoutExactASTMatch
, as theonlyCompareErrors
option has been pushed up to therunFixtureTests
API, so it can simply be called withtrue
as the last argument to get this behavior.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
andstartColumn
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
andfalse
(and missing). Given that tests are already generated with errorRecovery set totrue
, when we run it withfalse
, 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: #14146Generally 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.