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.

Sign up for GitHub

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

Jump to bottom

[Analyzer] Analyzer unit tests need updating for null safety release #43777

Closed
leafpetersen opened this issue Oct 13, 2020 · 18 comments
Closed

[Analyzer] Analyzer unit tests need updating for null safety release #43777

leafpetersen opened this issue Oct 13, 2020 · 18 comments
Assignees
@scheglov
Labels
area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. NNBD Issues related to NNBD Release P1 A high priority bug; for example, a single project is unusable or has many test failures

Comments

@leafpetersen
Copy link
Member

The analyzer has various unit tests that define and analyze Dart code. Currently, that code is treated as opted out of null safety. When the experiment flag is flipped, that code becomes treated as opted in, and some of it is no longer valid (it also arguable is no longer testing the same thing). This CL flips the experiment flag, and can be used to test this out. For a concrete example, see this log which shows a failure on pkg/analysis_server/test/analysis/reanalyze_test.dart (as well as others).

cc @scheglov @bwilkerson @devoncarew
@leafpetersen leafpetersen added area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. NNBD Issues related to NNBD Release labels Oct 13, 2020
@bwilkerson
Copy link
Member

If we simply update all of the tests to be valid under Null Safety then we will have very little coverage of legacy code. If that's a concern, then we might need to do something more complex. We haven't really worried about this in the past, but most of the other features that were enabled were additive while this one is breaking.

@leafpetersen
Copy link
Member Author

Keeping the legacy coverage seems useful. Ideally, there is a solution which doesn't require adding // @dart = 2.9 to every code snippet (e.g. setting the language version in the setup code in some way). Assuming this is possible, you could initially run all of the tests in legacy mode (keeping the existing coverage) and then either now or later port the unit tests to get additional coverage on null safe code, possibly running both a legacy and a ported set?

@scheglov
Copy link
Contributor

Yes, all our analyzer tests are now based on PubPackageResolutionTest, and we then mix WithNullSafetyMixin that updates analysis_options.yaml to enable the experiment. For the case when Null Safety is enabled by default, we could change PubPackageResolutionTest to write .dart_tool/package_config.json with 2.8 (?) as the language version for the test package, and WithNullSafetyMixin to set the language version to 2.12 (?). Then we will able to run in both modes, or migrate to null safety.

@franklinyow franklinyow added the type-enhancement A request for a change that isn't a bug label Oct 14, 2020
@franklinyow
Copy link
Contributor

Move this under the Analyzer Epic

@leafpetersen
Copy link
Member Author

Is someone working on this? We will be blocked on this in the not too distant future.

@scheglov scheglov mentioned this issue Oct 17, 2020
Closed
@scheglov
Copy link
Contributor

The update for analyzer/ is here: https://dart-review.googlesource.com/c/sdk/+/168240

It was relatively easy to do, because we switched analyzer/ to using .dart_tool/package_config.json, so we can control language versions.

But analysis_server/ is still using old .packages files, and not just for the package under test, but also to add dependencies. So, we will need to update all these places, possibly switch analysis_server/ also to PubPackageResolutionTest.

dart-bot pushed a commit that referenced this issue Oct 19, 2020
@scheglov
Issue 43777. Update analyzer/ tests to run when Null Safety is enable…
23b9cce 
…d by default in 2.12

Bug: #43777
Change-Id: If7e5f71fe1d8277898a02745f96f1c8003cc8069
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/168240
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
@franklinyow franklinyow removed the type-enhancement A request for a change that isn't a bug label Oct 19, 2020
@franklinyow
Copy link
Contributor

Move this back out to Beta2 epic, remove type-enhancement label

@scheglov scheglov self-assigned this Oct 20, 2020
@franklinyow franklinyow added the P1 A high priority bug; for example, a single project is unusable or has many test failures label Oct 20, 2020
@leafpetersen
Copy link
Member Author

Any updates on this?

@srawlins
Copy link
Member

@bwilkerson would you be able to look at this? I am not so familiar with analysis_server, but Konstantin mentions the fix may be to use PubPackageResolutionTest, which I am planning on using in my fix for #43883, the sibling bug for nnbd_migration.

@bwilkerson
Copy link
Member

Based on the comments in https://dart-review.googlesource.com/c/sdk/+/168920, I'm not sure what approach I should be implementing. But once we know what needs to be done I can assess how big the task is and whether I can take it on.

@srawlins
Copy link
Member

I was able to fix the nnbd_migration tests with a smaller arc. https://dart-review.googlesource.com/c/sdk/+/168900

The trick is to also write a package config file expressing the languageVersion of each package involved (possibly just one package, the one for the individual test file...).

@srawlins srawlins assigned srawlins and unassigned srawlins Oct 26, 2020
@scheglov
Copy link
Contributor

https://dart-review.googlesource.com/c/sdk/+/169141 for analysis_server/.

@srawlins
Copy link
Member

I believe there are 13 tests that turn red in analyzer_plugin. Grep for test_nodeInList_argumentList_only_named_trailingComma for example. I can look at these.

dart-bot pushed a commit that referenced this issue Oct 27, 2020
@scheglov
Issue 43777. Update analysis_server/ tests to run when Null Safety is…
d9d7324 
… enabled by default in 2.12

Bug: #43777
Change-Id: Ibd1da16b58b8304d6f1d4260615c7049fc5ffe59
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/169141
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Samuel Rawlins <srawlins@google.com>
@leafpetersen
Copy link
Member Author

Still seeing some failures here.

dart-bot pushed a commit that referenced this issue Oct 28, 2020
@srawlins
analyzer_plugin: Prepare tests for Null safety flag flip.
38e3414 
Only one test suite featured tests which needed to be in Null safety, so
that was split into two suites. Otherwise, the changes very nearly mirror
those in https://dart-review.googlesource.com/c/sdk/+/169141.


Bug: #43777
Change-Id: I39219dc0157ac33d4be1d902930060577e659cad
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/169244
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
@scheglov
Copy link
Contributor

Fixes for integration tests in analyzer/ and analysis_server/.
https://dart-review.googlesource.com/c/sdk/+/169286

@leafpetersen
Copy link
Member Author

Looks like we're down to these failures:

pkg/analysis_server/test/integration/analysis/highlights2_test
pkg/analysis_server/test/integration/analysis/highlights_test
pkg/analysis_server/test/integration/analysis/navigation_test
pkg/analysis_server/test/integration/edit/get_assists_test
pkg/analyzer/test/src/summary2/ast_text_printer_integration_test
pkg/analyzer_cli/test/driver_test

dart-bot pushed a commit that referenced this issue Oct 28, 2020
@scheglov
Fix integration tests for analyzer/ and analysis_server/ in preparati…
2e99e59 
…on for Null Safety turned on.

Bug: #43777
Change-Id: Ibc5426dd3035c54306bed6e0dec8a6b379163057
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/169286
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
@scheglov
Copy link
Contributor

https://dart-review.googlesource.com/c/sdk/+/169286 landed to fix analysis_server/ integration tests and ast_text_printer_integration_test.
https://dart-review.googlesource.com/c/sdk/+/169440 should fix analyzer_cli/.

dart-bot pushed a commit that referenced this issue Oct 28, 2020
@scheglov
Prepare analyzer_cli/ tests for turning Null Safety on by default.
2eef1aa 
Bug: #43777
Change-Id: I0699915c45369fac5590a6ada4a8ded186612947
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/169440
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
@leafpetersen
Copy link
Member Author

Looks green now, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@scheglov scheglov

Labels
area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. NNBD Issues related to NNBD Release P1 A high priority bug; for example, a single project is unusable or has many test failures
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@srawlins @scheglov @bwilkerson @leafpetersen @franklinyow