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.

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

Restructure analyzer test cases to be null safe by default #44666

Closed
srawlins opened this issue Jan 14, 2021 · 7 comments
Closed

Restructure analyzer test cases to be null safe by default #44666

srawlins opened this issue Jan 14, 2021 · 7 comments
Labels
analyzer-technical-debt area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. NNBD Issues related to NNBD Release P2 A bug or feature request we're likely to work on type-task

Comments

@srawlins
Copy link
Member

I don't know if anyone has vigorously updated our test files to include both a pre-null-safety and a post-null-safety class, even if there are no new or deprecated test cases for the new class. For example, non_constant_case_expression_test.dart contains:

main() {
  defineReflectiveSuite(() {
    defineReflectiveTests(NonConstantCaseExpressionTest);
    defineReflectiveTests(NonConstantCaseExpressionWithNullSafetyTest);
  });
}

@reflectiveTest
class NonConstantCaseExpressionTest extends PubPackageResolutionTest {
  // Test cases
}

@reflectiveTest
class NonConstantCaseExpressionWithNullSafetyTest
    extends NonConstantCaseExpressionTest with WithNullSafetyMixin {}

I'm happy to grind through "all" of our test files (maybe all diagnostics/ tests and a few other directories?) and add null safety sibling test classes.

In addition, maybe I'll take the step of defaulting to null safety (since it is the default language version now), and write classes like FooTest and FooWithoutNullSafetyTest.

@srawlins srawlins added type-task area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. NNBD Issues related to NNBD Release analyzer-technical-debt labels Jan 14, 2021
@srawlins
Copy link
Member Author

CC @bwilkerson @scheglov

@bwilkerson
Copy link
Member

I'll defer to Konstantin on this one, but we discussed doing just this when we started implementing null-safety and the consensus at that point is that we didn't need to duplicate the number of tests when they generally didn't depend on null-safety semantics. I think that was and still is the right decision.

Whether we want to reverse the names (and order in the hierarchy) of the test classes that have a WithNullSafety companion class is a whole other question. While it would keep the naming consistent moving into the future, it sounds like a lot of work with a questionable amount of benefit. I'd be inclined to leave it as is until the naming starts to become a burden.

@srawlins
Copy link
Member Author

... we didn't need to duplicate the number of tests when they generally didn't depend on null-safety semantics. I think that was and still is the right decision.

That makes sense to me. What about just default turning on null safety in all test classes that don't have a sibling WithNullSafety class?

@scheglov
Copy link
Contributor

We were adding WithNullSafety tests when there is a difference in resolving legacy and null safe code.
When there is no difference, it would be fine to run with null safety.
I have a migration CL for the analyzer, and it would be better if we avoided doing large scale changes before landing this CL to reduce amount of rebase.

@srawlins
Copy link
Member Author

I have a migration CL for the analyzer, and it would be better if we avoided doing large scale changes before landing this CL to reduce amount of rebase.

Haha don't worry, I'm keeping any lower-priority work in a holding pattern until that CL lands. Easier for me to rebase a small CL on a giant change than vice versa.

@scheglov scheglov added the P2 A bug or feature request we're likely to work on label Jan 20, 2021
@srawlins srawlins changed the title Add "WithNullSafety" sibling test classes for analyzer tests. Restructure analyzer test cases to be null safe by default Feb 4, 2021
dart-bot pushed a commit that referenced this issue Feb 7, 2021
Tests that include, for example, a field `int a;` need to be migrated to
something which is both legal in pre-null safety, and in null safety. So no `?`
etc,. Additionally:

* parameters to `main` must be correct,
* new flow analysis may change inference,
* avoid `List()`

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

scheglov commented Feb 8, 2021

One approach to reduce the number of combinations could be that the base of all tests (usually PubPackageResolutionTest) always uses the latest language version, and also all experimental features enabled. And then when we don't want some experiments - we write @dart = x.y in files of this test; or do writeTestPackageConfig.

Some tests we might want to run in both before-null-safety and such latest language with experiments modes. In this case we can continue to have two classes and XyzTestCases mixins.

@srawlins
Copy link
Member Author

srawlins commented Feb 8, 2021

I think this sounds good.

I doubt we will re-write the type system too many more times, and for smaller language features like non-function typedefs, triple-shift, annotations-with-type-args, etc, the vast majority of tests should work with and without the language feature. For the tests which are affected, individual care should be taken, such as multiple test classes.

dart-bot pushed a commit that referenced this issue Feb 8, 2021
Some tests had some issues migrating to null safety that I
could not immediately hash out, or may warrant some more discussion.
So I've added WithoutNullSafetyMixin, and we can incrementally
migrate these after this CL.

Many tests have little no-op tweaks to make them null safe, like
initializing variables, adding late, making nullable.

Nest steps:
1. Remove `WithNullSafetyMixin` applications.
2. Remove `WithoutNullSafetyMixin` in every position with a TODO for
   #44666.
3. For every file with pair(s) of test classes (one pre-null safety,
   and one null safety), move around test cases so that they generally
   run with null safety, unless they are testing things very specific to
   before and after null safety.

Bug: #44666
Change-Id: I0512dedcda42e864fd580af4842702095937bdb6
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/183140
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
dart-bot pushed a commit that referenced this issue Feb 9, 2021
In each included file, there is a test, and a WithNullSafety test which _does
not override_ any test cases from the base test. So the test classes are
combined into one, which is null safe. No test cases are added or removed, but
because of sorting, the diffs may be large.

Bug: #44666
Change-Id: Ia9bd2c512ccaa5b8de6fbe454c1275f1b8e889a9
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/183783
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
dart-bot pushed a commit that referenced this issue Feb 9, 2021
… no new test cases.

Bug: #44666
Change-Id: I4620377cb78a2e3d89e772322636a775bcfd8ba0
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/183782
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
dart-bot pushed a commit that referenced this issue Feb 9, 2021
Bug: #44666
Change-Id: I80135a4ae03820438547223938b1ee4dababdd7f
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/183820
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
dart-bot pushed a commit that referenced this issue Feb 10, 2021
…pedef experiment

This also deletes WithNonFunctionTypeAliasesMixin and all mixin
applications.

With how the tests are set up, I had to migrate a few other test cases
to use null safety. A few tests were written in pairs, one
pre-non-function-typedefs, and one post-. These should be combined, but
doing so in this CL would create a much larger diff, with the sorting.

Bug: #44666
Change-Id: Iee7f329ee8d841905f08249888de85256187d84b
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/184080
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
dart-bot pushed a commit that referenced this issue Feb 11, 2021
Since non-function typedefs are enabled by default with
PubPackageResolutionTest, these test classes are currently
unnecessarily split.

Bug: #44666
Change-Id: I0ab32931371fb917df11e45250c84528af9b9e2e
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/184206
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
dart-bot pushed a commit that referenced this issue Feb 13, 2021
In order to do this, I split the test into two:

* many test cases were concerned with type promotion
  which only seemed to pertain to pre-null safe code.
* some code could not be made valid in both null safe
  and pre-null safe environments.

Bug: #44666
Change-Id: I818a39f2e3eb3cae17de18b008d9e4c7b8ff84c5
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/184844
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
dart-bot pushed a commit that referenced this issue Feb 18, 2021
Bug: #44666
Change-Id: I3788f2bb7e7b082a2732a3a77fe59d3547f53f40
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/185484
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
dart-bot pushed a commit that referenced this issue Feb 19, 2021
I started trying to move checker_tests out from that very old test
class into invalid_assignment, and it was overwhelming. I rolled back
most of the work and kept this part.

Bug: #44666
Change-Id: Ic41b562cf263b4dae271de2da0f2ec5a716d6dfa
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/185491
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
dart-bot pushed a commit that referenced this issue Feb 19, 2021
All of the tests in dart2_inference_test work fine in null safety,
except a small handful. I moved these into more modern test locations,
and updated the tests to be simpler.

Much much much of the assignment tests were already covered in
assignment_test.dart. I just added a few test cases that did not have
the same coverage.

Bug: #44666
Change-Id: I144ad4aa7e839c47af0d24c3118ce7bd91eeca7c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/185724
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
dart-bot pushed a commit that referenced this issue Feb 19, 2021
This was mostly converting star expectations to nullability-suffix-none.

There was a little bit of expected dynamic -> Object conversions,
and expected Null -> void conversions.

Bug: #44666
Change-Id: Id064a398b8830a60552dfe57d3b5acc89f139eb8
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/185723
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
dart-bot pushed a commit that referenced this issue Feb 21, 2021
The diagnostics referred to in these tests are not reported in null safe
code.

Bug: #44666
Change-Id: I59676e2c937e154a519649754b2cb196313a3dd6
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/186120
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
dart-bot pushed a commit that referenced this issue Feb 22, 2021
…luation_test

Bug: #44666
Change-Id: Ic21b1f7ad03cf2049d1db672aa9e314da3dd3a49
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/186121
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
dart-bot pushed a commit that referenced this issue Feb 24, 2021
Bug: #44666
Change-Id: I7db26be235d3911b6c88b199df874da1ee2d552e
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/186760
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
dart-bot pushed a commit that referenced this issue Feb 24, 2021
…test classes

In each file, test cases were simply moved, and not changed at all. No test
cases are removed. None are added.

Bug: #44666
Change-Id: I7566602f53a234b34e3dc54176ac9b80139f444b
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/186761
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
dart-bot pushed a commit that referenced this issue Feb 24, 2021
…esolutionTest

Bug: #44666
Change-Id: I01aa8a5908b8a2f195859c4c9fd99e3127249e29
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/186880
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
copybara-service bot pushed a commit that referenced this issue Nov 28, 2023
Related to #44666

I was able to combine a few of these with their modern equivalents,
where test names did not clash, but in general these tests can just be
removed: These tests test specifics about the pre-NNBD type system;
assignability, promotability, etc.

Change-Id: I735ccaf7f29b828c0891c18c87e9d9bc1279e52a
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/338428
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer-technical-debt area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. NNBD Issues related to NNBD Release P2 A bug or feature request we're likely to work on type-task
Projects
None yet
Development

No branches or pull requests

3 participants