[XL] Change in dart/sdk[main]: [ddc] Porting VM hot reload tests to the hot reload framework related...

0 views
Skip to first unread message

Nicholas Shahan (Gerrit)

unread,
Oct 29, 2024, 7:59:56 PM10/29/24
to Mark Zhou, Commit Queue, rev...@dartlang.org
Attention needed from Mark Zhou

Nicholas Shahan added 3 comments

File tests/hot_reload/delete_static_field/main.1.dart
Line 20, Patchset 3 (Latest): return field.value;
Nicholas Shahan . unresolved

It doesn't feel like this is testing anything regarding the deletion of the static field since the access after is not through the static field at all. That happened in the first generation when it still existed.

I see that the existing test can't really be copied 1:1 so we have to try to figure out what they are testing.

What does the VM do if we tearoff a function that accesses Foo.x in it's body and then call the tearoff in the generation where the static field has been removed?

File tests/hot_reload/existing_static_field_changes_type_indirect_function/main.0.dart
Line 35, Patchset 3 (Latest): "type '(A) => bool' is not a subtype of type '(B) => bool' of 'function "
"result'",
Nicholas Shahan . unresolved

Does the TypeError message from DDC match this? If not we might need to get more creative in how we test for the expected error in both DDC and the VM.

File tests/hot_reload/existing_static_field_changes_type_indirect_generic/main.0.dart
Line 33, Patchset 3 (Latest): "type 'List<B>' is not a subtype of type 'List<A>' of 'function result'",
Nicholas Shahan . unresolved

ditto

Open in Gerrit

Related details

Attention is currently required from:
  • Mark Zhou
Submit Requirements:
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: sdk
Gerrit-Branch: main
Gerrit-Change-Id: I3c42781acd253ac82658593c96ab92ef6c2cdb50
Gerrit-Change-Number: 392466
Gerrit-PatchSet: 3
Gerrit-Owner: Mark Zhou <mark...@google.com>
Gerrit-Reviewer: Mark Zhou <mark...@google.com>
Gerrit-Reviewer: Nicholas Shahan <nsh...@google.com>
Gerrit-Attention: Mark Zhou <mark...@google.com>
Gerrit-Comment-Date: Tue, 29 Oct 2024 23:59:53 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
unsatisfied_requirement
open
diffy

Mark Zhou (Gerrit)

unread,
Oct 31, 2024, 5:31:34 PM10/31/24
to Nicholas Shahan, Commit Queue, rev...@dartlang.org
Attention needed from Nicholas Shahan

Mark Zhou added 3 comments

File tests/hot_reload/delete_static_field/main.1.dart
Line 20, Patchset 3: return field.value;
Nicholas Shahan . unresolved

It doesn't feel like this is testing anything regarding the deletion of the static field since the access after is not through the static field at all. That happened in the first generation when it still existed.

I see that the existing test can't really be copied 1:1 so we have to try to figure out what they are testing.

What does the VM do if we tearoff a function that accesses Foo.x in it's body and then call the tearoff in the generation where the static field has been removed?

Mark Zhou

My best guess is that they're checking for "intended" memory leaks (counting allocated heap objects).

With a tearoff, it yields
`
NoSuchMethodError: No static getter 'x' declared in class 'null'.
`

That might be the only DDC-relevant side effect of deleting (then accessing) a static field. I can capture that behavior here and leave the memory checking out. Thoughts?

File tests/hot_reload/existing_static_field_changes_type_indirect_function/main.0.dart
Line 35, Patchset 3: "type '(A) => bool' is not a subtype of type '(B) => bool' of 'function "
"result'",
Nicholas Shahan . resolved

Does the TypeError message from DDC match this? If not we might need to get more creative in how we test for the expected error in both DDC and the VM.

Mark Zhou

Good call. We check substring matches, so I'll just remove the portion that we don't emit. Luckily, that's just the 'result' part. If we see something more complex, I'm inclined to unify our errors (or get clever).

File tests/hot_reload/existing_static_field_changes_type_indirect_generic/main.0.dart
Line 33, Patchset 3: "type 'List<B>' is not a subtype of type 'List<A>' of 'function result'",
Nicholas Shahan . resolved

ditto

Mark Zhou

Done

Open in Gerrit

Related details

Attention is currently required from:
  • Nicholas Shahan
Submit Requirements:
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: sdk
Gerrit-Branch: main
Gerrit-Change-Id: I3c42781acd253ac82658593c96ab92ef6c2cdb50
Gerrit-Change-Number: 392466
Gerrit-PatchSet: 5
Gerrit-Owner: Mark Zhou <mark...@google.com>
Gerrit-Reviewer: Mark Zhou <mark...@google.com>
Gerrit-Reviewer: Nicholas Shahan <nsh...@google.com>
Gerrit-Attention: Nicholas Shahan <nsh...@google.com>
Gerrit-Comment-Date: Thu, 31 Oct 2024 21:31:30 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nicholas Shahan <nsh...@google.com>
unsatisfied_requirement
open
diffy

Nicholas Shahan (Gerrit)

unread,
Nov 6, 2024, 2:17:22 PM11/6/24
to Mark Zhou, Commit Queue, rev...@dartlang.org
Attention needed from Mark Zhou

Nicholas Shahan voted and added 7 comments

Votes added by Nicholas Shahan

Code-Review+1

7 comments

File tests/hot_reload/not_typedef_to_typedef/main.0.dart
File tests/hot_reload/run_new_field_initializers_cyclic_initialization/main.0.dart
Line 28, Patchset 3: Expect.throws(() => helper());
Nicholas Shahan . unresolved

Do we expect both backends will throw a StackOverflowException and should we narrow this expectation?

File tests/hot_reload/run_new_field_initializers_super_class/main.0.dart
Line 28, Patchset 3: helper();
Nicholas Shahan . unresolved

An expectation here might make the update more clear.

File tests/hot_reload/run_new_field_initializers_throws/main.0.dart
Line 28, Patchset 3: Expect.throws(() => helper());
Nicholas Shahan . unresolved

Should validate the thrown exception is what we actually expect.

File tests/hot_reload/run_new_field_initializers_with_generics/main.2.dart
Line 36, Patchset 3: Expect.type<Map<String, String>>(v1);
Expect.type<Map<int, int>>(v2);
Nicholas Shahan . unresolved

Is there a reason these got moved into a third generation compared to the original test?

File tests/hot_reload/typedef_add_parameter/main.0.dart
File tests/hot_reload/typedef_to_not_typedef/main.0.dart
Open in Gerrit

Related details

Attention is currently required from:
  • Mark Zhou
Submit Requirements:
  • requirement satisfiedCode-Review
  • requirement satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: sdk
Gerrit-Branch: main
Gerrit-Change-Id: I3c42781acd253ac82658593c96ab92ef6c2cdb50
Gerrit-Change-Number: 392466
Gerrit-PatchSet: 6
Gerrit-Owner: Mark Zhou <mark...@google.com>
Gerrit-Reviewer: Mark Zhou <mark...@google.com>
Gerrit-Reviewer: Nicholas Shahan <nsh...@google.com>
Gerrit-Attention: Mark Zhou <mark...@google.com>
Gerrit-Comment-Date: Wed, 06 Nov 2024 19:17:16 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
satisfied_requirement
open
diffy

Mark Zhou (Gerrit)

unread,
Nov 7, 2024, 5:28:54 PM11/7/24
to Nicholas Shahan, Commit Queue, rev...@dartlang.org

Mark Zhou voted Commit-Queue+2

Commit-Queue+2
Open in Gerrit

Related details

Attention set is empty
Submit Requirements:
  • requirement satisfiedCode-Review
  • requirement satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: sdk
Gerrit-Branch: main
Gerrit-Change-Id: I3c42781acd253ac82658593c96ab92ef6c2cdb50
Gerrit-Change-Number: 392466
Gerrit-PatchSet: 8
Gerrit-Owner: Mark Zhou <mark...@google.com>
Gerrit-Reviewer: Mark Zhou <mark...@google.com>
Gerrit-Reviewer: Nicholas Shahan <nsh...@google.com>
Gerrit-Comment-Date: Thu, 07 Nov 2024 22:28:51 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
satisfied_requirement
open
diffy

Commit Queue (Gerrit)

unread,
Nov 7, 2024, 6:13:41 PM11/7/24
to Mark Zhou, Nicholas Shahan, rev...@dartlang.org

Commit Queue submitted the change with unreviewed changes

Unreviewed changes

6 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:

```
The name of the file: tests/hot_reload/run_new_field_initializers_cyclic_initialization/main.0.dart
Insertions: 1, Deletions: 1.

The diff is too large to show. Please review the diff.
```
```
The name of the file: tests/hot_reload/run_new_field_initializers_super_class/main.0.dart
Insertions: 2, Deletions: 2.

The diff is too large to show. Please review the diff.
```
```
The name of the file: tests/hot_reload/existing_static_field_changes_type_indirect_function/main.0.dart
Insertions: 1, Deletions: 1.

The diff is too large to show. Please review the diff.
```
```
The name of the file: tests/hot_reload/typedef_add_parameter/main.0.dart
Insertions: 1, Deletions: 1.

The diff is too large to show. Please review the diff.
```
```
The name of the file: tests/hot_reload/typedef_to_not_typedef/main.0.dart
Insertions: 1, Deletions: 1.

The diff is too large to show. Please review the diff.
```
```
The name of the file: tests/hot_reload/run_new_field_initializers_cyclic_initialization/main.1.dart
Insertions: 1, Deletions: 1.

The diff is too large to show. Please review the diff.
```
```
The name of the file: tests/hot_reload/existing_static_field_changes_type_indirect_function/main.1.dart
Insertions: 1, Deletions: 1.

The diff is too large to show. Please review the diff.
```
```
The name of the file: tests/hot_reload/run_new_field_initializers_super_class/main.1.dart
Insertions: 2, Deletions: 2.

The diff is too large to show. Please review the diff.
```
```
The name of the file: tests/hot_reload/not_typedef_to_typedef/main.0.dart
Insertions: 1, Deletions: 1.

The diff is too large to show. Please review the diff.
```
```
The name of the file: tests/hot_reload/typedef_to_not_typedef/main.1.dart
Insertions: 1, Deletions: 1.

The diff is too large to show. Please review the diff.
```
```
The name of the file: tests/hot_reload/typedef_add_parameter/main.1.dart
Insertions: 2, Deletions: 2.

The diff is too large to show. Please review the diff.
```
```
The name of the file: tests/hot_reload/not_typedef_to_typedef/main.1.dart
Insertions: 1, Deletions: 1.

The diff is too large to show. Please review the diff.
```
```
The name of the file: tests/hot_reload/run_new_field_initializers_throws/main.1.dart
Insertions: 3, Deletions: 3.

The diff is too large to show. Please review the diff.
```
```
The name of the file: tests/hot_reload/run_new_field_initializers_throws/main.0.dart
Insertions: 1, Deletions: 1.

The diff is too large to show. Please review the diff.
```

Change information

Commit message:
[ddc] Porting VM hot reload tests to the hot reload framework related to type updates.
Change-Id: I3c42781acd253ac82658593c96ab92ef6c2cdb50
Commit-Queue: Mark Zhou <mark...@google.com>
Reviewed-by: Nicholas Shahan <nsh...@google.com>
Files:
  • A tests/hot_reload/delete_static_field/config.json
  • A tests/hot_reload/delete_static_field/main.0.dart
  • A tests/hot_reload/delete_static_field/main.1.dart
  • A tests/hot_reload/existing_field_changes_type/config.json
  • A tests/hot_reload/existing_field_changes_type/main.0.dart
  • A tests/hot_reload/existing_field_changes_type/main.1.dart
  • M tests/hot_reload/existing_field_changes_type_indirect/main.0.dart
  • M tests/hot_reload/existing_field_changes_type_indirect/main.1.dart
  • M tests/hot_reload/existing_field_changes_type_indirect_function/main.0.dart
  • M tests/hot_reload/existing_field_changes_type_indirect_function/main.1.dart
  • M tests/hot_reload/existing_field_changes_type_indirect_generic/main.0.dart
  • M tests/hot_reload/existing_field_changes_type_indirect_generic/main.1.dart
  • M tests/hot_reload/existing_static_field_changes_type/main.0.dart
  • M tests/hot_reload/existing_static_field_changes_type/main.1.dart
  • M tests/hot_reload/existing_static_field_changes_type_indirect/main.0.dart
  • M tests/hot_reload/existing_static_field_changes_type_indirect/main.1.dart
  • A tests/hot_reload/existing_static_field_changes_type_indirect_function/config.json
  • A tests/hot_reload/existing_static_field_changes_type_indirect_function/main.0.dart
  • A tests/hot_reload/existing_static_field_changes_type_indirect_function/main.1.dart
  • A tests/hot_reload/existing_static_field_changes_type_indirect_generic/config.json
  • A tests/hot_reload/existing_static_field_changes_type_indirect_generic/main.0.dart
  • A tests/hot_reload/existing_static_field_changes_type_indirect_generic/main.1.dart
  • A tests/hot_reload/not_typedef_to_typedef/config.json
  • A tests/hot_reload/not_typedef_to_typedef/main.0.dart
  • A tests/hot_reload/not_typedef_to_typedef/main.1.dart
  • A tests/hot_reload/run_new_field_initializers_cyclic_initialization/config.json
  • A tests/hot_reload/run_new_field_initializers_cyclic_initialization/main.0.dart
  • A tests/hot_reload/run_new_field_initializers_cyclic_initialization/main.1.dart
  • A tests/hot_reload/run_new_field_initializers_super_class/config.json
  • A tests/hot_reload/run_new_field_initializers_super_class/main.0.dart
  • A tests/hot_reload/run_new_field_initializers_super_class/main.1.dart
  • A tests/hot_reload/run_new_field_initializers_throws/config.json
  • A tests/hot_reload/run_new_field_initializers_throws/main.0.dart
  • A tests/hot_reload/run_new_field_initializers_throws/main.1.dart
  • A tests/hot_reload/run_new_field_initializers_with_generics/config.json
  • A tests/hot_reload/run_new_field_initializers_with_generics/main.0.dart
  • A tests/hot_reload/run_new_field_initializers_with_generics/main.1.dart
  • A tests/hot_reload/run_new_field_initializers_with_generics/main.2.dart
  • A tests/hot_reload/super_class_changed/config.json
  • A tests/hot_reload/super_class_changed/main.0.dart
  • A tests/hot_reload/super_class_changed/main.1.dart
  • A tests/hot_reload/super_getter_rebound_to_method/config.json
  • A tests/hot_reload/super_getter_rebound_to_method/main.0.dart
  • A tests/hot_reload/super_getter_rebound_to_method/main.1.dart
  • A tests/hot_reload/type_identity/config.json
  • A tests/hot_reload/type_identity/main.0.dart
  • A tests/hot_reload/type_identity/main.1.dart
  • A tests/hot_reload/type_identity_generic/config.json
  • A tests/hot_reload/type_identity_generic/main.0.dart
  • A tests/hot_reload/type_identity_generic/main.1.dart
  • A tests/hot_reload/type_identity_parameter/config.json
  • A tests/hot_reload/type_identity_parameter/main.0.dart
  • A tests/hot_reload/type_identity_parameter/main.1.dart
  • A tests/hot_reload/typedef_add_parameter/config.json
  • A tests/hot_reload/typedef_add_parameter/main.0.dart
  • A tests/hot_reload/typedef_add_parameter/main.1.dart
  • A tests/hot_reload/typedef_to_not_typedef/config.json
  • A tests/hot_reload/typedef_to_not_typedef/main.0.dart
  • A tests/hot_reload/typedef_to_not_typedef/main.1.dart
Change size: XL
Delta: 59 files changed, 1283 insertions(+), 22 deletions(-)
Branch: refs/heads/main
Submit Requirements:
  • requirement satisfiedCode-Review: +1 by Nicholas Shahan
Open in Gerrit
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: merged
Gerrit-Project: sdk
Gerrit-Branch: main
Gerrit-Change-Id: I3c42781acd253ac82658593c96ab92ef6c2cdb50
Gerrit-Change-Number: 392466
Gerrit-PatchSet: 9
Gerrit-Owner: Mark Zhou <mark...@google.com>
open
diffy
satisfied_requirement

Mark Zhou (Gerrit)

unread,
Nov 22, 2024, 11:14:28 AM11/22/24
to Commit Queue, Nicholas Shahan, rev...@dartlang.org

Mark Zhou added 7 comments

File tests/hot_reload/not_typedef_to_typedef/main.0.dart
Nicholas Shahan . resolved

Link is misaligned?

Mark Zhou

Done

File tests/hot_reload/run_new_field_initializers_cyclic_initialization/main.0.dart
Line 28, Patchset 3: Expect.throws(() => helper());
Nicholas Shahan . resolved

Do we expect both backends will throw a StackOverflowException and should we narrow this expectation?

Mark Zhou

Done

File tests/hot_reload/run_new_field_initializers_super_class/main.0.dart
Nicholas Shahan . resolved

An expectation here might make the update more clear.

Mark Zhou

Done

File tests/hot_reload/run_new_field_initializers_throws/main.0.dart
Line 28, Patchset 3: Expect.throws(() => helper());
Nicholas Shahan . resolved

Should validate the thrown exception is what we actually expect.

Mark Zhou

I added a string check here, which should be sufficient.

File tests/hot_reload/run_new_field_initializers_with_generics/main.2.dart
Line 36, Patchset 3: Expect.type<Map<String, String>>(v1);
Expect.type<Map<int, int>>(v2);
Nicholas Shahan . resolved

Is there a reason these got moved into a third generation compared to the original test?

Mark Zhou

It was just easier to express as a 2-tuple vs a 2-tuple then 4-tuple. No major reason - lemme know if you'd prefer it looking more similar.

File tests/hot_reload/typedef_add_parameter/main.0.dart
Nicholas Shahan . resolved

Link is misaligned.

Mark Zhou

Done

File tests/hot_reload/typedef_to_not_typedef/main.0.dart
Nicholas Shahan . resolved

Link is misaligned.

Mark Zhou

Done

Open in Gerrit

Related details

Attention set is empty
Submit Requirements:
  • requirement satisfiedCode-Review
  • requirement satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: sdk
Gerrit-Branch: main
Gerrit-Change-Id: I3c42781acd253ac82658593c96ab92ef6c2cdb50
Gerrit-Change-Number: 392466
Gerrit-PatchSet: 9
Gerrit-Owner: Mark Zhou <mark...@google.com>
Gerrit-Reviewer: Mark Zhou <mark...@google.com>
Gerrit-Reviewer: Nicholas Shahan <nsh...@google.com>
Gerrit-Comment-Date: Fri, 22 Nov 2024 16:14:25 +0000
satisfied_requirement
open
diffy
Reply all
Reply to author
Forward
0 new messages