docgen: Replace fixtures with inline parsed code. #40428
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dmsnell
force-pushed
the
docgen/replace-fixtures-with-code
branch
3 times, most recently
from
ae9829a to
13e5361
Compare
|
Size Change: 0 B Total Size: 1.23 MB ℹ️ View Unchanged
|
sarayourfriend
approved these changes
Apr 19, 2022
There was a problem hiding this comment.
I think you're correct to skip the line numbers, they're not actually used by the rest of the code as far as I can tell.
Assertion module: null seems wise though, given it does vary and is significant:
lineStart and lineEnd, while explicitly added, are probably more helpful for debugging than anything.
Love this clean up!
Comment on lines
112
to
128
| expect( | ||
| getTypeAnnotation( { tag: 'param', name: 'foo' }, node, 0 ) | ||
| ).toBe( literal ); | ||
| expect( getTypeAnnotation( { tag: 'return' }, node ) ).toBe( | ||
| literal | ||
| ); |
There was a problem hiding this comment.
It'd be nice for these to be separate tests so they fail separate from each other (the code that handles them is relatively separate as well). It'd be annoying for this test to fail on the first assertion, fix it, only to find out that it was also failing on the second, mostly unrelated assertion.
Previously in the `docgen` package test suite we relied on developers manually creating a set of files for each test that includes a reference snippet of code, a parsed AST, and an exports listing. These files weren't linked together in any way. In this patch we're removing the manual steps and inlining the code snippets inside each test in the suite. This removes a level of manual work and indirection when maintaining the tests while leaning more on `docgen`'s `engine` component, which itself does the actual parsing.
dmsnell
force-pushed
the
docgen/replace-fixtures-with-code
branch
from
159b126 to
08784cb
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Previously in the
docgenpackage test suite we relied on developers manually creatinga set of files for each test that includes a reference snippet of code, a parsed AST,
and an exports listing. These files weren't linked together in any way.
In this patch we're removing the manual steps and inlining the code snippets inside each
test in the suite. This removes a level of manual work and indirection when maintaining
the tests while leaning more on
docgen'senginecomponent, which itself does theactual parsing.
Why?
We don't want to have to depend on people remembering do every manual step on every change in order to hold together the assurances our tests provide. In the course of refactoring this code, for example, there were several places where we were testing ASTs that didn't correspond to the code they purported to represent.
After this change we won't have the opportunities to create those mismatches and the false sense of safety they provide.
How?
This patch removes what amounts to duplicate copies of test data. By narrowing to a single source of truth and using the
docgenfunctionality directly from inside the tests we are giving ourselves a double-assurance that everything works as we want it to. Because the new single source of truth stands inline directly in each test we've removed a layer of indirection when trying to understand or modify the tests and their assertions.Testing Instructions
There are no built-bundle changes in this PR. There are only test refactors. Please audit the test changes and consider the refactored approach: specifically these things:
parse()andfirstToken()test helpers an acceptable level of abstraction within test code? While relatively shallow in their functionality they allow us to focus our tests on the code and the expected results. Without them we would be adding a fair amount of noise within the tests.code.jsas written over preserving the AST representations)expect.objectContaining()gives us a lot of flexibility in focusing on the values we want to assert and ignore the incidentals. Of note, line numbers were already a source of discrepancy between the test code fixtures and the AST fixtures.module: nulldo we need this or can we remove that constraint?