Fixes #2496. Fix co19 tests that became failing after test runner update #2497

sgrekhov · 2024-01-18T14:26:03Z

Please don't merge this PR until landing of https://dart-review.googlesource.com/c/sdk/+/345541

eernstg

LGTM.

Waiting for https://dart-review.googlesource.com/c/sdk/+/345541.

Language/Libraries_and_Scripts/definition_syntax_t18.dart

davidmorgan · 2024-01-19T10:26:27Z

Language/Libraries_and_Scripts/Parts/part_0.dart

@@ -10,7 +10,4 @@ class Annot {
 }

 final foo = "foo";
-//    ^^^
-// [analyzer] unspecified
-// [cfe] unspecified


Per discussion on

dart-lang/sdk#54661

I think this failure assertion is correct when analyzing compilation_t01.dart and incorrect when analyzing compilation_t04.dart. We have no way to assert failure in one and not the other, so I think the right fix is to copy part_0.dart into a separate version with no failure assertion as in my PR.

I may be wrong, as I commented on the issue there is a lot of complexity here ;)

Yes, this change seems wrong. Let's wait the conclusion in dart-lang/sdk#54661

At this point I think we have a plan: Change all tests involving parts such that each part has a uniquely determined owner (in co19, we can't stop anyone "out there" who insists on owning one of these parts). We would then create 4 copies of this part file and use each of them as a part in compilation_t0n.dart for n in {1, 2, 3, 4}. This would eliminate the contradictory expectations.

I think this was done already?

sgrekhov · 2024-02-07T08:29:23Z

I prepared a change that fixes all of the new test failures introduced by the change in the test runner. In most cases I simply don't expect analyzer error there. Updated file looks like

// TODO (sgrekhov) Add expected analyzer error below. For now (Feb, 2024)
// analyzer doesn't analyze parts, this error is not reported by the analyzer
// and this is not an error but an intended behavior.
// See https://github.com/dart-lang/sdk/issues/54661

part of assert;
//      ^^^^^^
// [cfe] unspecified

This change will fix the current errors. When the analyzer will be fixed (at least in the test runner) to analyze the parts I'll remove TODO and add error expectations.

I was going to submit this change, but according to the latest comment

For the test runner I expect that it would be natural to report every diagnostic from every reachable file (which would be trivial to implement with what we have today.

If the fix of the test runner is a trivial may be it makes sense to wait for the change first and then fix the tests?

The third option is to allow the test to be failing for now until the test runner will be fixed. Just to add issue number

// @issue 54661

part of assert;
//      ^^^^^^
// [analyzer] unspecified
// [cfe] unspecified

@davidmorgan @eernstg what do you think? Which option is better?

eernstg · 2024-02-07T09:26:30Z

About the test runner reporting diagnostics from all entities (libraries and parts) that are being analyzed: That sounds like a really nice solution! I'm pushing for that here.

So let's try to clarify this proposal a bit more, and then the answers to the questions about part of assert; etc. might follow automatically.

eernstg

Added a couple of comments about things that may be resolved at this time.

Language/Libraries_and_Scripts/definition_syntax_t18.dart

eernstg · 2024-02-07T09:11:16Z

Language/Libraries_and_Scripts/Parts/part_0.dart

@@ -10,7 +10,4 @@ class Annot {
 }

 final foo = "foo";
-//    ^^^
-// [analyzer] unspecified
-// [cfe] unspecified


At this point I think we have a plan: Change all tests involving parts such that each part has a uniquely determined owner (in co19, we can't stop anyone "out there" who insists on owning one of these parts). We would then create 4 copies of this part file and use each of them as a part in compilation_t0n.dart for n in {1, 2, 3, 4}. This would eliminate the contradictory expectations.

davidmorgan · 2024-02-13T15:16:23Z

Getting back to this: my PR for the test_runner is about ready, I'll send it out for review. https://dart-review.googlesource.com/c/sdk/+/345541 ... It now adds e2e tests so you can see that expectations for the analyzer are picked up for imported libraries and included parts.

You can see the co19 test failures here https://dart-ci.firebaseapp.com/cl/345541/8 ... a question @eernstg @sgrekhov should I aim to land it with approvals for those failures? I think another option would be to land these co19 test fixes first and roll them into the SDK, as my understand is they should pass with both versions of the test runner.

Thanks!

sgrekhov · 2024-02-14T08:19:09Z

It's our common practice to fill co19 issue and then to approve the failures. And I fix the failing tests later. So, for me it's ok simply to approve the failures

eernstg · 2024-02-14T08:59:54Z

@davidmorgan wrote:

land it with approvals for those failures?

Sounds good! Usually, a test failure approval comment will mention an issue where the situation is described and any remaining tasks are tracked. In this case I guess that would be an issue on the analyzer-as-test_runner, requesting that it should report all compile-time errors from the transitive closure of each of the analysis targets. Do we have an issue like that? This comment seems to be really close. ;-)

sgrekhov · 2024-02-14T09:21:27Z

co19 issue #2496

davidmorgan · 2024-02-14T15:30:14Z

Aaand the test_runner change is in! Indeed I pointed to #2496 in approving the failures.

sgrekhov · 2024-02-14T15:31:54Z

Thank you! Cool! I'll fix the failures

eernstg

I find it a little difficult to see whether everything is done here. How about we land this, and then we can try to include it in a roll and see if it is working? ;-)

@davidmorgan, @sgrekhov, WDYT?

eernstg · 2024-02-14T17:08:09Z

Language/Libraries_and_Scripts/definition_syntax_t18.dart

-/// is missing the library name.
+///
+/// @description Checks that it is not an error if a library name is missing in
+/// the library directive


Suggested change

/// the library directive

/// the library directive of the imported library.

Language/Libraries_and_Scripts/definition_syntax_t18.dart

eernstg · 2024-02-14T17:11:13Z

Language/Libraries_and_Scripts/Parts/part_0.dart

@@ -10,7 +10,4 @@ class Annot {
 }

 final foo = "foo";
-//    ^^^
-// [analyzer] unspecified
-// [cfe] unspecified


I think this was done already?

sgrekhov · 2024-02-14T17:38:45Z

@eernstg I'm going to update this PR tomorrow and send it to review. So, let's wait till tomorrow

eernstg · 2024-02-14T17:44:13Z

Very good, thanks!

…unner update

sgrekhov · 2024-02-15T11:08:29Z

Updated. All tests that marked as failing here https://dart-ci.firebaseapp.com/cl/345541/10 became Ok. Erik's notes also implemented. Please review

Note that https://dart-review.googlesource.com/c/sdk/+/345541 is still not landed yet

davidmorgan · 2024-02-15T11:10:17Z

Whoops, I thought it was merged, thanks for pointing that out :)

eernstg

LGTM!

eernstg

LGTM.

davidmorgan · 2024-02-20T12:28:24Z

Okay, the test_runner change is actually merged now. Thanks again and sorry for the confusion caused last time :)

2024-02-20 [email protected] dart-lang/co19#2496. Add missing part file (dart-lang/co19#2552) 2024-02-20 [email protected] dart-lang/co19#2549. Update built-in identifier tests. Add missing ones. Part 2 (dart-lang/co19#2551) 2024-02-20 [email protected] dart-lang/co19#2549. Update built-in identifier tests. Add missing ones. Part 1 (dart-lang/co19#2550) 2024-02-19 [email protected] Fix dart-lang/co19#2546. Add constant context tests (dart-lang/co19#2547) 2024-02-15 [email protected] Fixes dart-lang/co19#2496. Fix co19 tests that became failing after test runner update (dart-lang/co19#2497) 2024-02-14 [email protected] dart-lang/co19#1400. Add cascade operator test and more constants tests (dart-lang/co19#2545) 2024-02-14 [email protected] dart-lang/co19#2350. Add more factory constructor tests (dart-lang/co19#2543) 2024-02-14 [email protected] dart-lang/co19#1400. Add representation variable initialization test (dart-lang/co19#2544) 2024-02-13 [email protected] dart-lang/co19#2119. Remove unsupported element from Stream.first/lastWhere tests description (dart-lang/co19#2540) 2024-02-13 [email protected] dart-lang/co19#2119. Remove unsupported element from ReceivePort.first/lastWhere tests description (dart-lang/co19#2541) 2024-02-13 [email protected] dart-lang/co19#2339. Add more tests for the extension method with the name `type` (dart-lang/co19#2542) 2024-02-12 [email protected] Fix dart-lang/co19#2535. Add patterns constants tests for extension types (dart-lang/co19#2539) 2024-02-12 [email protected] Fix dart-lang/co19#2485. Update`as` and `is` expressions tests with a function type (dart-lang/co19#2538) 2024-02-12 [email protected] Fix dart-lang/co19#2536. Fix use of HINT.UNREACHABLE_SWITCH_CASE (dart-lang/co19#2537) 2024-02-09 [email protected] dart-lang/co19#2119. Remove unused code, add issues numbers (dart-lang/co19#2534) 2024-02-09 [email protected] dart-lang/co19#2529. Fix failing API core tests (dart-lang/co19#2533) 2024-02-09 [email protected] dart-lang/co19#2529. Delete API tests with compile-time errors (dart-lang/co19#2531) 2024-02-09 [email protected] dart-lang/co19#2529. Fix and enable Iterable/forEach_A03_t01 (dart-lang/co19#2532) Change-Id: I03e721d3871d5bf2b9773fbf7ebadb2a1804a111 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/353260 Reviewed-by: Alexander Thomas <[email protected]> Reviewed-by: Erik Ernst <[email protected]> Commit-Queue: Alexander Thomas <[email protected]>

sgrekhov requested review from chloestefantsova, davidmorgan and eernstg January 18, 2024 14:26

sgrekhov mentioned this pull request Jan 18, 2024

Fixes for pending change to test runner. #2494

Closed

eernstg approved these changes Jan 18, 2024

View reviewed changes

davidmorgan reviewed Jan 19, 2024

View reviewed changes

eernstg reviewed Feb 7, 2024

View reviewed changes

davidmorgan mentioned this pull request Feb 14, 2024

[test_runner] test.py should presumably support errors in a non-entry-point library dart-lang/sdk#44990

Closed

eernstg approved these changes Feb 14, 2024

View reviewed changes

sgrekhov marked this pull request as draft February 15, 2024 10:07

sgrekhov force-pushed the fixes-for-test-runner-change branch 2 times, most recently from c2ac670 to 0de9928 Compare February 15, 2024 10:54

Fixes dart-lang#2496. Fix co19 tests that became failing after test r…

2f5eb57

…unner update

sgrekhov force-pushed the fixes-for-test-runner-change branch from 0de9928 to 2f5eb57 Compare February 15, 2024 11:02

Additional fix that actually fixes failing tests

58b69c0

sgrekhov marked this pull request as ready for review February 15, 2024 11:08

sgrekhov requested review from eernstg and davidmorgan February 15, 2024 11:08

eernstg approved these changes Feb 15, 2024

View reviewed changes

eernstg merged commit 8a066d4 into dart-lang:master Feb 15, 2024
1 of 2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #2496. Fix co19 tests that became failing after test runner update #2497

Fixes #2496. Fix co19 tests that became failing after test runner update #2497

sgrekhov commented Jan 18, 2024

eernstg left a comment

davidmorgan Jan 19, 2024

sgrekhov Jan 19, 2024 •

edited

Loading

eernstg Feb 7, 2024

eernstg Feb 14, 2024

sgrekhov commented Feb 7, 2024

eernstg commented Feb 7, 2024

eernstg left a comment

eernstg Feb 7, 2024

davidmorgan commented Feb 13, 2024

sgrekhov commented Feb 14, 2024 •

edited

Loading

eernstg commented Feb 14, 2024

sgrekhov commented Feb 14, 2024

davidmorgan commented Feb 14, 2024

sgrekhov commented Feb 14, 2024

eernstg left a comment

eernstg Feb 14, 2024

eernstg Feb 14, 2024

sgrekhov commented Feb 14, 2024

eernstg commented Feb 14, 2024

sgrekhov commented Feb 15, 2024

davidmorgan commented Feb 15, 2024

eernstg left a comment

eernstg left a comment

davidmorgan commented Feb 20, 2024

	/// the library directive
	/// the library directive of the imported library.

Fixes #2496. Fix co19 tests that became failing after test runner update #2497

Fixes #2496. Fix co19 tests that became failing after test runner update #2497

Conversation

sgrekhov commented Jan 18, 2024

eernstg left a comment

Choose a reason for hiding this comment

davidmorgan Jan 19, 2024

Choose a reason for hiding this comment

sgrekhov Jan 19, 2024 • edited Loading

Choose a reason for hiding this comment

eernstg Feb 7, 2024

Choose a reason for hiding this comment

eernstg Feb 14, 2024

Choose a reason for hiding this comment

sgrekhov commented Feb 7, 2024

eernstg commented Feb 7, 2024

eernstg left a comment

Choose a reason for hiding this comment

eernstg Feb 7, 2024

Choose a reason for hiding this comment

davidmorgan commented Feb 13, 2024

sgrekhov commented Feb 14, 2024 • edited Loading

eernstg commented Feb 14, 2024

sgrekhov commented Feb 14, 2024

davidmorgan commented Feb 14, 2024

sgrekhov commented Feb 14, 2024

eernstg left a comment

Choose a reason for hiding this comment

eernstg Feb 14, 2024

Choose a reason for hiding this comment

eernstg Feb 14, 2024

Choose a reason for hiding this comment

sgrekhov commented Feb 14, 2024

eernstg commented Feb 14, 2024

sgrekhov commented Feb 15, 2024

davidmorgan commented Feb 15, 2024

eernstg left a comment

Choose a reason for hiding this comment

eernstg left a comment

Choose a reason for hiding this comment

davidmorgan commented Feb 20, 2024

sgrekhov Jan 19, 2024 •

edited

Loading

sgrekhov commented Feb 14, 2024 •

edited

Loading