Skip to content

Move analyzer test utilities to analyzer_utilities package #55660

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

Open
Tracked by #60036
srawlins opened this issue May 7, 2024 · 3 comments
Open
Tracked by #60036

Move analyzer test utilities to analyzer_utilities package #55660

srawlins opened this issue May 7, 2024 · 3 comments
Assignees
Labels
area-dart-model For issues related to conformance to the language spec in the parser, compilers or the CLI analyzer. P2 A bug or feature request we're likely to work on type-code-health Internal changes to our tools and workflows to make them cleaner, simpler, or more maintainable

Comments

@srawlins
Copy link
Member

srawlins commented May 7, 2024

Quick background / example: I would like to move ResolveNameInScopeTest from the analyzer package to the linter package. But it depends on AbstractLinterContextTest (also in pkg/analyzer/test) which depends on PubPackageResolutionTest (also in pkg/analyzer/test). So I cannot move any tests until we have some shared base classes. Or I can copy swaths of code.

Creating shared testing code between the analyzer package, linter package, analysis_server and analysis_server_plugin packages is necessary for the plugins story. CC @bwilkerson @scheglov @pq

ContextResolutionTest

It would take significant investigation to determine how many test utility classes and mixins would be prudent to move. But as a starting place, I think it would be of huge benefit to move ContextResolutionTest into package:analyzer_utilities. How do we get there?

ResourceProviderMixin

ResourceProviderMixin (mixed into ContextResolutionTest) is declared in package:analyzer/src/test_utilities, so we're already in a good place there. Should be easier to move than other things, as it cannot depend on code in pkg/analyzer/test. This might be the best starting place.

ResolutionTest

ContextResolutionTest also mixes in ResolutionTest, so this is also a pre-requirement. Unfortunately it lives in pkg/analyzer/test, and is in a file which imports eight other files from pkg/analyzer/test. Yikes.

PubPackageResolutionTest

PubPackageResolutionTest is also an important base class, as it is the base for supporting analysis options files and package configs.

@srawlins srawlins added legacy-area-analyzer Use area-devexp instead. type-code-health Internal changes to our tools and workflows to make them cleaner, simpler, or more maintainable P2 A bug or feature request we're likely to work on labels May 7, 2024
@bwilkerson
Copy link
Member

It's quite possible that some of the base classes might include support that isn't needed by the tests that a plugin author would need to write. It might be worthwhile to start by thinking about what the requirements are and what those pieces of support actually depend on. Moving the rest of the support out to mixins might reduce the number of dependencies.

It's also the case that plugin authors will need to be able to do some things that we currently don't have support for.

For example, here are some initial thoughts.

Plugin authors need to be able to test both lints and fixes.

For both of those use cases the tests need to be able to create the code being tested and any supporting code in an in-memory disk layout. That includes being able to create analysis options files, package config files, and any (mock) packages that the test code needs to be able to depend on (including packages only available to the plugin author).

For lint tests they need to be able to specify which file(s) to analyze and the diagnostics that the lint is expected to produce. They'll also need to be able to register the lint rule being tested.

For fix tests they need to do every they need for testing lint rules as well as confirming the result of applying the fix to the appropriate files.

They probably don't need the printers that we use for printing resolution output, the findNode and findElement utilities, and a reasonable portion of the assert methods, among other things.

Also, are you thinking of analyzer_utilities as a stepping stone to a published package of plugin test utilities, or are you thinking that we'd eventually publish analyzer_utilities? I'd prefer the former, but you might know of a reason why that isn't the best option.

@srawlins
Copy link
Member Author

srawlins commented May 7, 2024

Also, are you thinking of analyzer_utilities as a stepping stone to a published package of plugin test utilities, or are you thinking that we'd eventually publish analyzer_utilities?

Either. I hadn't thought of the differences.

Something that may make this task difficult is that much of ContextResolutionTest (and others I mentioned) depends on package:analyzer. It uses MemoryByteStore, AnalysisContextCollection, MockSdkLibrary, Folder, etc. I can't remember the cyclic dependency rules with certainty. But I think that two packages cannot depend on each other (directly or indirectly) through their dependencies. Two packages can only depend on each other if at least one of the dependencies is via dev_dependencies. And I think that also means that at least one of the dependency chains must involve a dev_dependency instead of a dependency, yes?

Anyways, I think that means that analyzer_utilities can have a dependency on analyzer, but then analyzer cannot have a dependency on analyzer_utilities, meaning that no code in pkg/analyzer/lib/src/test_utilities can depend on package:analyzer_utilities. That code would have to move first.

@srawlins
Copy link
Member Author

Another anecdote pointing to the need to solve this:

I recently moved selection.dart from analysis_server to analysis_server_plugin. @bwilkerson pointed out I should move the test as well, selection_test.dart. When I tried doing this, I saw that this would require moving text_expectations.dart as well (or copying it). But text_expectations.dart (TextExpectationsCollector) is required by half a dozen other tests in analysis_server. So I could copy that over to analysis_server_plugin, in it's lib/ directory (something like lib/src/test_utilities) but then the analysis_server_plugin package would need a dependency (not a dev_dependency) on both package:test and packag:test_reflective_loader. ☹️ I think this is super smelly and not adhering to the purpose of the analysis_server_plugin package.

So I think we need an analysis_server_plugin_testing (or analyzer_testing or pick your better name) package. Above, @bwilkerson wrote:

Also, are you thinking of analyzer_utilities as a stepping stone to a published package of plugin test utilities, or are you thinking that we'd eventually publish analyzer_utilities? I'd prefer the former [...]

I wrote, "Either. I hadn't thought of the differences." Now I'm thinking a fresh, test-oriented package (like, the name is also test-oriented) is warranted. We wouldn't need to publish this package for a while. Maybe not until plugin authors need it.

@johnniwinther johnniwinther added area-dart-model For issues related to conformance to the language spec in the parser, compilers or the CLI analyzer. and removed legacy-area-analyzer Use area-devexp instead. labels Mar 5, 2025
@srawlins srawlins self-assigned this Apr 29, 2025
copybara-service bot pushed a commit that referenced this issue Apr 29, 2025
Work towards #55660

Change-Id: Ia4f8186c328c9929869d1305ee0fe606d9dfa0e5
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/425320
Commit-Queue: Samuel Rawlins <[email protected]>
Reviewed-by: Brian Wilkerson <[email protected]>
Reviewed-by: Alexander Thomas <[email protected]>
copybara-service bot pushed a commit that referenced this issue May 2, 2025
We intend to publish and maintain this as a set of testing-related
utilities for the analyzer packages and for analyzer plugins.

Work towards #55660

See the doc: https://docs.google.com/document/d/1jRtd8B1ijPAP6Pz89HRnyIZXw2VMjaZx0vRZTpoNO84/edit?tab=t.0#heading=h.2sz41a544qhi

Change-Id: I2764b1357a932fa955060b26d78038997eaa9536
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/425080
Reviewed-by: Slava Egorov <[email protected]>
Commit-Queue: Samuel Rawlins <[email protected]>
Reviewed-by: Paul Berry <[email protected]>
copybara-service bot pushed a commit that referenced this issue May 5, 2025
Only one test implementation used it, and it can be a proper
function instead of a getter that returns a function.

Work towards #55660

Change-Id: I9f6e6d4c6f88c0b613c69abec6b714ef5fe4bf64
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/426401
Auto-Submit: Samuel Rawlins <[email protected]>
Reviewed-by: Brian Wilkerson <[email protected]>
Commit-Queue: Brian Wilkerson <[email protected]>
copybara-service bot pushed a commit that referenced this issue May 6, 2025
Work towards #55660

Change-Id: Ic789b8347d3bfc71a02af423fbb1a8c58da7b32d
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/424921
Reviewed-by: Ivan Inozemtsev <[email protected]>
Commit-Queue: Samuel Rawlins <[email protected]>
Reviewed-by: Konstantin Shcheglov <[email protected]>
Reviewed-by: Brian Wilkerson <[email protected]>
copybara-service bot pushed a commit that referenced this issue May 7, 2025
Work towards #55660

The comment for `MemoryResourceProvider.convertPath` includes:

> This is a utility method for testing; paths passed in to other
> methods in this class are never converted automatically.

and indeed, the actual impl of this method is found in an extension,
in analyzer's test_utilities/ directory. It seems to me better to
leave a testing utility as a testing utility, and not expose it in
the public API of MemoryResourceProvider.

Additionally, the extension method is used by `ResourceProviderMixin`,
which is moving to the public analyzer_testing package. It is illegal
to have a circular non-dev dependency between the analyzer package
and the analyzer_testing package.

The migration for the ~half dozen test files that use this method is
to call the extension method directly. Sometimes with an extension
override, and sometimes without.

Change-Id: I9c2e18600461134bfd91c082b3af0d5079600f0c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/426984
Commit-Queue: Samuel Rawlins <[email protected]>
Reviewed-by: Konstantin Shcheglov <[email protected]>
Reviewed-by: Brian Wilkerson <[email protected]>
copybara-service bot pushed a commit that referenced this issue May 7, 2025
Work towards #55660

Change-Id: I9c5aa920e9f96f99ea00d6993b5f4dafd415f831
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/426986
Reviewed-by: Konstantin Shcheglov <[email protected]>
Commit-Queue: Samuel Rawlins <[email protected]>
copybara-service bot pushed a commit that referenced this issue May 9, 2025
Work towards #55660

Change-Id: I3df2cd374f6b3ef9e027f0e07c748be6ea21ebb2
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/427586
Reviewed-by: Paul Berry <[email protected]>
Commit-Queue: Samuel Rawlins <[email protected]>
copybara-service bot pushed a commit that referenced this issue May 9, 2025
Work towards #55660

Change-Id: I31932409e495369793dd16c017385c98b86cf44c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/427480
Reviewed-by: Paul Berry <[email protected]>
Commit-Queue: Samuel Rawlins <[email protected]>
copybara-service bot pushed a commit that referenced this issue May 12, 2025
Work towards #55660

Spelunker will move to the analyzer_testing package, so we must first
clean it up.

Spelunker had two subclasses, but their only differentiation was the
calculation of a getter, which could be a final field. So I remove the
subclasses, move the "File"-based one to the spelunk utility script,
and change the Spelunker constructor to take in a source string.
Additionally:

* Make all of the fields private.
* Remove isDartFileName and isPubspecFileName. These calculations can
  be inlined into their call sites and make use of the `file_paths`
  library.

Change-Id: I4b97ed03a2845440dabd3a9ab0aa3b3ba4af5959
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/428083
Reviewed-by: Konstantin Shcheglov <[email protected]>
Commit-Queue: Samuel Rawlins <[email protected]>
copybara-service bot pushed a commit that referenced this issue May 12, 2025
Work towards #55660

The only direct subclass of `_ContextResolutionTest` is
`PubPackageResolutionTest`, so they can be combined. I keep all of the
API the same, with a few exceptions:

* Make `addTestFile`, `testFilePath`, `resolveTestFile` private. They
  were very lightly used outside of `rule_test_support.dart`, in
  exceptional situations, where they can be replaced with local members.
* This allows for a much more idiomatic impl for `file_names_test.dart`.

Change-Id: Icfa63ca488c4722067fc7d471343afd398a9dddc
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/428161
Reviewed-by: Brian Wilkerson <[email protected]>
Commit-Queue: Samuel Rawlins <[email protected]>
copybara-service bot pushed a commit that referenced this issue May 14, 2025
Work towards #55660

Change-Id: I5ffe612695be725e0af0497f9b2917da3d52ec7a
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/428420
Auto-Submit: Samuel Rawlins <[email protected]>
Reviewed-by: Brian Wilkerson <[email protected]>
Commit-Queue: Brian Wilkerson <[email protected]>
copybara-service bot pushed a commit that referenced this issue May 15, 2025
Work towards #55660

Change-Id: Iae062d95e5cf3b94b540496242b3627268ac2612
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/428844
Commit-Queue: Samuel Rawlins <[email protected]>
Reviewed-by: Paul Berry <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-dart-model For issues related to conformance to the language spec in the parser, compilers or the CLI analyzer. P2 A bug or feature request we're likely to work on type-code-health Internal changes to our tools and workflows to make them cleaner, simpler, or more maintainable
Projects
None yet
Development

No branches or pull requests

3 participants