FEDX-522 Add APIs to help test suggestors with resolved AST #74
Conversation
evanweible-wf
requested review from
greglittlefield-wf,
robbecker-wf and
sorenthompson-wk
Security InsightsNo security relevant content was detected by automated scans. Action Items
Questions or Comments? Reach out on Slack: #support-infosec. |
| .toList() | ||
| .then((patches) => applyPatches(context.sourceFile, patches)), | ||
| completion(resultMatcher)); | ||
| class PackageContextForTest { |
There was a problem hiding this comment.
This looks like a slimmed down version of a similar utility in over_react_codemod; I like it! https://github.com/Workiva/over_react_codemod/blob/bb73b48f65e0e4fa4ee53f21549a74f0f45351c7/test/resolved_file_context.dart#L46
There was a problem hiding this comment.
I knew I should have looked harder in that package 😆
lib/test.dart
Outdated
| /// that require the resolved AST. | ||
| /// | ||
| /// See [PackageContextForTest] for an example. | ||
| Future<FileContext> addFile(String path, String sourceText) async { |
There was a problem hiding this comment.
In over_react_codemod, we ended up having to share contexts across tests, since it would take too long to spin up a new analysis context for each test.
With that setup, it's a bit annoying to have to come up with unique file names for each test case, so being able to auto-generate them is nice (and what we ended up doing in over_react_codemod).
There was a problem hiding this comment.
Yeah I was wondering about that.. I was about to type something out about how it's unfortunate that the test_descriptor package uses addTearDown by default since that effectively scopes everything it creates to a single test, but then I found this issue in that repo for that exact case:
https://github.com/dart-lang/test_descriptor/issues/11
And it turns out they solved it by updating how addTearDown from the test package works so that it knows when is called from setUpAll, which is awesome! TIL! So that means consumers could create a PackageContextForTest in a setUpAll to share it across tests without any other changes here. With that in mind, I agree, auto-generating a file name would be convenient. I had wondered about auto-generating a name for the directory as well, so I think I'll do that, too.
There was a problem hiding this comment.
Awesome, thanks for adding that!
Oh nice, I was wondering how if the sharing might not be possible test_descriptor; glad that works with setUpAll!
evanweible-wf
dismissed stale reviews from robbecker-wf and sorenthompson-wk
via
5330958
robbecker-wf
changed the title
|
@Workiva/release-management-p |
rmconsole4-wk
deleted the
add-test-apis-for-testing-resolved-ast-suggestors
branch
Motivation
AstVisitingSuggestormakes it easy to write suggestors that resolve the AST by overridingshouldResolveAst(), but we don't make it very easy to test those suggestors, especially on files that have imports.Changes
Add
PackageContextForTestto help with this use case. See readme for details.