[L] Change in dart/sdk[main]: Convert more lint tests to use markdown
Brian Wilkerson (Gerrit)
Brian Wilkerson voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Rawlins (Gerrit)
Samuel Rawlins voted and added 2 comments![]( "Open in Gerrit")
Votes added by Samuel Rawlins
| Code-Review | +1 |
2 comments
await assertNoDiagnosticsInFile(b.path);This asserts that `augment f();` doesn't have diagnostics. I think this is an important expectation to keep, no?
augment class A {I think we should assert that the lint is not fired here.
Related details
- Brian Wilkerson
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brian Wilkerson (Gerrit)
Brian Wilkerson added 2 comments![]( "Open in Gerrit")
await assertNoDiagnosticsInFile(b.path);This asserts that `augment f();` doesn't have diagnostics. I think this is an important expectation to keep, no?
I didn't think so. We have lots of tests that use two or more files, and we almost never test that the other files don't have diagnostics. I'm not sure why this would be any different.
augment class A {I think we should assert that the lint is not fired here.
I don't understand why this would be a valuable thing to do. Could you explain your thoughts?
Related details
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Rawlins (Gerrit)
Samuel Rawlins added 3 comments![]( "Open in Gerrit")
await assertNoDiagnosticsInFile(b.path);Brian WilkersonThis asserts that `augment f();` doesn't have diagnostics. I think this is an important expectation to keep, no?
I didn't think so. We have lots of tests that use two or more files, and we almost never test that the other files don't have diagnostics. I'm not sure why this would be any different.
That's fair, as long as we have another test here that asserts that `always_declare_return_types` isn't reported on an augmentation, an asserting that is being removed from this test case in this CL.
augment class A {Brian WilkersonI think we should assert that the lint is not fired here.
I don't understand why this would be a valuable thing to do. Could you explain your thoughts?
Oops I misunderstood the test; augmenting a class with an instance field is not an interesting case. Nevermind.
augment class A {I think we should assert that the lint is or is not fired here.
If an `augment class` declaration only declares static fields, we should make sure that the lint rule does not report it, or does report it, according to some design.
I'm not sure what we've decided for this case. But it seems to me that the lint rule should decide whether to report lint based on the "totality" of class, the initial declaration and all augmentations, so that it never needs to report on individual augmentations, and would only ever report on the initial declaration.
But if that's not an interesting case to you, disregard.
Related details
- Brian Wilkerson
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brian Wilkerson (Gerrit)
Brian Wilkerson added 1 comment![]( "Open in Gerrit")
augment class A {I think we should assert that the lint is or is not fired here.
If an `augment class` declaration only declares static fields, we should make sure that the lint rule does not report it, or does report it, according to some design.
I'm not sure what we've decided for this case. But it seems to me that the lint rule should decide whether to report lint based on the "totality" of class, the initial declaration and all augmentations, so that it never needs to report on individual augmentations, and would only ever report on the initial declaration.
But if that's not an interesting case to you, disregard.
I'm not sure what we've decided for this case.
I don't think we've decided anything. This was added by Konstantin as part of adding augmentation support to the analyzer. I'm not really sure why he did this.
But I agree with your take on it. I think that any diagnostic associated with a declaration (like this one) should be reported at the first fragment and nowhere else.
I think we should assert that the lint is or is not fired here.
I'm wondering whether we should make this part of the general testing framework: track the created files and ensure that any `.dart` file not explicitly checked has no diagnostics associated with it. Not sure how we'd do that; it's probably not expected that `tearDown` will fail.
Related details
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Rawlins (Gerrit)
Samuel Rawlins added 1 comment![]( "Open in Gerrit")
augment class A {Brian WilkersonI think we should assert that the lint is or is not fired here.
If an `augment class` declaration only declares static fields, we should make sure that the lint rule does not report it, or does report it, according to some design.
I'm not sure what we've decided for this case. But it seems to me that the lint rule should decide whether to report lint based on the "totality" of class, the initial declaration and all augmentations, so that it never needs to report on individual augmentations, and would only ever report on the initial declaration.
But if that's not an interesting case to you, disregard.
I'm not sure what we've decided for this case.
I don't think we've decided anything. This was added by Konstantin as part of adding augmentation support to the analyzer. I'm not really sure why he did this.
But I agree with your take on it. I think that any diagnostic associated with a declaration (like this one) should be reported at the first fragment and nowhere else.
I think we should assert that the lint is or is not fired here.
I'm wondering whether we should make this part of the general testing framework: track the created files and ensure that any `.dart` file not explicitly checked has no diagnostics associated with it. Not sure how we'd do that; it's probably not expected that `tearDown` will fail.
- Brian Wilkerson
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brian Wilkerson (Gerrit)
Brian Wilkerson voted and added 2 comments![]( "Open in Gerrit")
Votes added by Brian Wilkerson
| Commit-Queue | +1 |
2 comments
await assertNoDiagnosticsInFile(b.path);Brian WilkersonThis asserts that `augment f();` doesn't have diagnostics. I think this is an important expectation to keep, no?
Samuel RawlinsI didn't think so. We have lots of tests that use two or more files, and we almost never test that the other files don't have diagnostics. I'm not sure why this would be any different.
That's fair, as long as we have another test here that asserts that `always_declare_return_types` isn't reported on an augmentation, an asserting that is being removed from this test case in this CL.
Given that the intent of this CL was to change the APIs being used and not to change the tests, I've gone back through and restored all of the assertions that were deleted.
augment class A {I think we should assert that the lint is not fired here.
Samuel RawlinsI don't understand why this would be a valuable thing to do. Could you explain your thoughts?
Oops I misunderstood the test; augmenting a class with an instance field is not an interesting case. Nevermind.
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brian Wilkerson (Gerrit)
Brian Wilkerson voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
dart-scoped@luci-project-accounts.iam.gserviceaccount.com (Gerrit)
dart-...@luci-project-accounts.iam.gserviceaccount.com submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
3 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: pkg/linter/test/rules/unnecessary_getters_setters_test.dart
Insertions: 4, Deletions: 2.
@@ -78,7 +78,7 @@
@FailingTest(issue: 'https://github.com/dart-lang/linter/issues/4935')
test_unnecessary_augmentationAddedGetterAndSetter() async {
- newFile('$testPackageLibPath/b.dart', r'''
+ var b = newFile('$testPackageLibPath/b.dart', r'''
part of 'test.dart';
augment class A {
@@ -97,11 +97,12 @@
class [!A!] {}
''');
+ await assertNoDiagnosticsInFile(b.path);
}
@FailingTest(issue: 'https://github.com/dart-lang/linter/issues/4935')
test_unnecessary_augmentationAddedSetter() async {
- newFile('$testPackageLibPath/b.dart', r'''
+ var b = newFile('$testPackageLibPath/b.dart', r'''
part of 'test.dart';
augment class A {
@@ -120,6 +121,7 @@
String? get [!x!] => _x;
}
''');
+ await assertNoDiagnosticsInFile(b.path);
}
test_unnecessary_getterAndSetter_extensionType() async {
```
```
The name of the file: pkg/linter/test/rules/overridden_fields_test.dart
Insertions: 5, Deletions: 2.
@@ -19,7 +19,7 @@
String get lintRule => LintNames.overridden_fields;
test_augmentationClass() async {
- newFile('$testPackageLibPath/a.dart', r'''
+ var a = newFile('$testPackageLibPath/a.dart', r'''
part 'test.dart';
class O {
@@ -36,10 +36,12 @@
final [!a!] = '';
}
''');
+ await assertNoDiagnosticsInFile(a.path);
}
+ @FailingTest(reason: 'There is a diagnostic in b.dart.')
test_augmentedField() async {
- newFile('$testPackageLibPath/b.dart', r'''
+ var b = newFile('$testPackageLibPath/b.dart', r'''
part of 'test.dart';
augment class A {
@@ -59,6 +61,7 @@
final [!a!] = '';
}
''');
+ await assertNoDiagnosticsInFile(b.path);
}
test_conflictingFieldAndMethod() async {
```
```
The name of the file: pkg/linter/test/rules/type_annotate_public_apis_test.dart
Insertions: 18, Deletions: 8.
@@ -19,7 +19,7 @@
String get lintRule => LintNames.type_annotate_public_apis;
test_augmentationClass_field() async {
- newFile('$testPackageLibPath/a.dart', r'''
+ var a = newFile('$testPackageLibPath/a.dart', r'''
part 'test.dart';
class A { }
@@ -32,10 +32,11 @@
var [!i!];
}
''');
+ await assertNoDiagnosticsInFile(a.path);
}
test_augmentationClass_method() async {
- newFile('$testPackageLibPath/a.dart', r'''
+ var a = newFile('$testPackageLibPath/a.dart', r'''
part 'test.dart';
class A { }
@@ -48,10 +49,11 @@
void f([!x!]) { }
}
''');
+ await assertNoDiagnosticsInFile(a.path);
}
test_augmentationTopLevelFunction() async {
- newFile('$testPackageLibPath/a.dart', r'''
+ var a = newFile('$testPackageLibPath/a.dart', r'''
part 'test.dart';
''');
@@ -60,10 +62,11 @@
void f([!x!]) { }
''');
+ await assertNoDiagnosticsInFile(a.path);
}
test_augmentationTopLevelVariable() async {
- newFile('$testPackageLibPath/a.dart', r'''
+ var a = newFile('$testPackageLibPath/a.dart', r'''
part 'test.dart';
''');
@@ -72,10 +75,12 @@
var [!x!];
''');
+ await assertNoDiagnosticsInFile(a.path);
}
+ @FailingTest(reason: 'There is a diagnostic in b.dart.')
test_augmentedField() async {
- newFile('$testPackageLibPath/b.dart', r'''
+ var b = newFile('$testPackageLibPath/b.dart', r'''
part of 'test.dart';
augment class A {
@@ -90,10 +95,11 @@
var [!x!];
}
''');
+ await assertNoDiagnosticsInFile(b.path);
}
test_augmentedMethod() async {
- newFile('$testPackageLibPath/b.dart', r'''
+ var b = newFile('$testPackageLibPath/b.dart', r'''
part of 'test.dart';
augment class A {
@@ -108,10 +114,11 @@
void f([!x!]) { }
}
''');
+ await assertNoDiagnosticsInFile(b.path);
}
test_augmentedTopLevelFunction() async {
- newFile('$testPackageLibPath/b.dart', r'''
+ var b = newFile('$testPackageLibPath/b.dart', r'''
part of 'test.dart';
augment void f(x);
@@ -122,10 +129,12 @@
void f([!x!]) { }
''');
+ await assertNoDiagnosticsInFile(b.path);
}
+ @FailingTest(reason: 'There is a diagnostic in b.dart.')
test_augmentedTopLevelVariable() async {
- newFile('$testPackageLibPath/b.dart', r'''
+ var b = newFile('$testPackageLibPath/b.dart', r'''
part of 'test.dart';
augment var x;
@@ -136,6 +145,7 @@
var [!x!];
''');
+ await assertNoDiagnosticsInFile(b.path);
}
test_enumConstructor_parameterMissingType() async {
```
```
The name of the file: pkg/linter/test/rules/avoid_classes_with_only_static_members_test.dart
Insertions: 6, Deletions: 3.
@@ -19,7 +19,7 @@
test_augmentationClass_nonStaticField() async {
// The added field should prevent a lint in 'test.dart'.
- newFile('$testPackageLibPath/b.dart', r'''
+ var b = newFile('$testPackageLibPath/b.dart', r'''
part of 'test.dart';
augment class A {
@@ -34,10 +34,11 @@
static int f = 1;
}
''');
+ await assertNoDiagnosticsInFile(b.path);
}
test_augmentationClass_staticField() async {
- newFile('$testPackageLibPath/b.dart', r'''
+ var b = newFile('$testPackageLibPath/b.dart', r'''
part of 'test.dart';
augment class A {
@@ -50,10 +51,11 @@
class [!A!] {}
''');
+ await assertNoDiagnosticsInFile(b.path);
}
test_augmentationClass_staticMethod() async {
- newFile('$testPackageLibPath/b.dart', r'''
+ var b = newFile('$testPackageLibPath/b.dart', r'''
part of 'test.dart';
augment class A {
@@ -66,6 +68,7 @@
class [!A!] {}
''');
+ await assertNoDiagnosticsInFile(b.path);
}
test_basicClass() async {
```
```
The name of the file: pkg/linter/test/rules/avoid_annotating_with_dynamic_test.dart
Insertions: 12, Deletions: 6.
@@ -18,7 +18,7 @@
String get lintRule => LintNames.avoid_annotating_with_dynamic;
test_augmentationClass() async {
- newFile('$testPackageLibPath/a.dart', r'''
+ var a = newFile('$testPackageLibPath/a.dart', r'''
part 'test.dart';
class A { }
@@ -31,10 +31,11 @@
void f([!dynamic o!]) { }
}
''');
+ await assertNoDiagnosticsInFile(a.path);
}
test_augmentationTopLevelFunction() async {
- newFile('$testPackageLibPath/a.dart', r'''
+ var a = newFile('$testPackageLibPath/a.dart', r'''
part 'test.dart';
''');
@@ -43,10 +44,11 @@
void f([!dynamic o!]) { }
''');
+ await assertNoDiagnosticsInFile(a.path);
}
test_augmentationTopLevelFunction_localDynamic() async {
- newFile('$testPackageLibPath/a.dart', r'''
+ var a = newFile('$testPackageLibPath/a.dart', r'''
part 'test.dart';
void f(int i);
@@ -60,10 +62,11 @@
g(i);
}
''');
+ await assertNoDiagnosticsInFile(a.path);
}
test_augmentedMethod() async {
- newFile('$testPackageLibPath/b.dart', r'''
+ var b = newFile('$testPackageLibPath/b.dart', r'''
part of 'test.dart';
augment class A {
@@ -78,10 +81,11 @@
void f([!dynamic o!]) { }
}
''');
+ await assertNoDiagnosticsInFile(b.path);
}
test_augmentedTopLevelFunction() async {
- newFile('$testPackageLibPath/b.dart', r'''
+ var b = newFile('$testPackageLibPath/b.dart', r'''
part of 'test.dart';
augment void f(dynamic o);
@@ -92,10 +96,11 @@
void f([!dynamic o!]) { }
''');
+ await assertNoDiagnosticsInFile(b.path);
}
test_augmentedTopLevelFunction_multiple() async {
- newFile('$testPackageLibPath/b.dart', r'''
+ var b = newFile('$testPackageLibPath/b.dart', r'''
part of 'test.dart';
augment void f(dynamic o);
@@ -107,6 +112,7 @@
void f([!dynamic o!]) { }
''');
+ await assertNoDiagnosticsInFile(b.path);
}
test_fieldFormals() async {
```
```
The name of the file: pkg/linter/test/rules/always_declare_return_types_test.dart
Insertions: 10, Deletions: 5.
@@ -18,7 +18,7 @@
String get lintRule => LintNames.always_declare_return_types;
test_augmentationClass() async {
- newFile('$testPackageLibPath/a.dart', r'''
+ var a = newFile('$testPackageLibPath/a.dart', r'''
part 'test.dart';
class A { }
@@ -31,10 +31,11 @@
[!f!]() { }
}
''');
+ await assertNoDiagnosticsInFile(a.path);
}
test_augmentationTopLevelFunction() async {
- newFile('$testPackageLibPath/a.dart', r'''
+ var a = newFile('$testPackageLibPath/a.dart', r'''
part 'test.dart';
''');
@@ -43,12 +44,13 @@
[!f!]() { }
''');
+ await assertNoDiagnosticsInFile(a.path);
}
/// Augmentation target chain variations tested in
/// `augmentedTopLevelFunction{*}`.
test_augmentedMethod() async {
- newFile('$testPackageLibPath/b.dart', r'''
+ var b = newFile('$testPackageLibPath/b.dart', r'''
part of 'test.dart';
augment class A {
@@ -63,10 +65,11 @@
[!f!]() { }
}
''');
+ await assertNoDiagnosticsInFile(b.path);
}
test_augmentedTopLevelFunction() async {
- newFile('$testPackageLibPath/b.dart', r'''
+ var b = newFile('$testPackageLibPath/b.dart', r'''
part of 'test.dart';
augment f();
@@ -77,10 +80,11 @@
[!f!]() { }
''');
+ await assertNoDiagnosticsInFile(b.path);
}
test_augmentedTopLevelFunction_chain() async {
- newFile('$testPackageLibPath/b.dart', r'''
+ var b = newFile('$testPackageLibPath/b.dart', r'''
part of 'test.dart';
augment dynamic f();
@@ -92,6 +96,7 @@
[!f!]() { }
''');
+ await assertNoDiagnosticsInFile(b.path);
}
test_extensionMethod() async {
```
```
The name of the file: pkg/linter/test/rules/prefer_typing_uninitialized_variables_test.dart
Insertions: 6, Deletions: 2.
@@ -17,8 +17,9 @@
@override
String get lintRule => LintNames.prefer_typing_uninitialized_variables;
+ @FailingTest(reason: 'There is a diagnostic in b.dart.')
test_field_augmented() async {
- newFile('$testPackageLibPath/b.dart', r'''
+ var b = newFile('$testPackageLibPath/b.dart', r'''
part of 'test.dart';
augment class A {
@@ -33,6 +34,7 @@
var [!x!];
}
''');
+ await assertNoDiagnosticsInFile(b.path);
}
test_field_final_noInitializer() async {
@@ -111,8 +113,9 @@
''');
}
+ @FailingTest(reason: 'There is a diagnostic in b.dart.')
test_topLevelVariable_augmented() async {
- newFile('$testPackageLibPath/b.dart', r'''
+ var b = newFile('$testPackageLibPath/b.dart', r'''
part of 'test.dart';
augment var x;
@@ -123,6 +126,7 @@
var [!x!];
''');
+ await assertNoDiagnosticsInFile(b.path);
}
test_topLevelVariable_var_initializer() async {
```
Change information
Convert more lint tests to use markdown
This was largely written by AI.
This converts most of the rest of the lint rule tests to use markdown
rather than hand-written offsets. None of the tests have been changed
in any other way, though it's sometimes hard to tell because the order
in which the files are written sometimes changed.
In addition, I found several skipped tests that no longer needed to
be skipped, so I removed the annotation. There are skipped tests in
some otherwise untouched files, but I left them skipped.
There are still tests in `analyzer_public_api_test.dart` that have not
been converted. These tests are organized by the API being tested rather
than the rule being tested. As a result they don't really fit the
general test framework and are harder to convert. I still want to
convert them, but I didn't want to do it in this CL because I thought
it would make it harder to review this CL.
- M pkg/linter/test/rule_test_support.dart
- M pkg/linter/test/rules/always_declare_return_types_test.dart
- M pkg/linter/test/rules/avoid_annotating_with_dynamic_test.dart
- M pkg/linter/test/rules/avoid_classes_with_only_static_members_test.dart
- M pkg/linter/test/rules/avoid_relative_lib_imports_test.dart
- M pkg/linter/test/rules/avoid_web_libraries_in_flutter_test.dart
- M pkg/linter/test/rules/depend_on_referenced_packages_test.dart
- M pkg/linter/test/rules/file_names_test.dart
- M pkg/linter/test/rules/overridden_fields_test.dart
- M pkg/linter/test/rules/prefer_typing_uninitialized_variables_test.dart
- M pkg/linter/test/rules/simple_directive_paths_test.dart
- M pkg/linter/test/rules/type_annotate_public_apis_test.dart
- M pkg/linter/test/rules/unnecessary_getters_setters_test.dart
- M pkg/linter/test/rules/use_enums_test.dart
Code-Review: +1 by Samuel Rawlins
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |