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

Preapproving dart2js and ddc NNBD failures for https://dart-review.googlesource.com/c/sdk/+/141252 #41275

Closed
liamappelbe opened this issue Mar 31, 2020 · 4 comments
Assignees
Labels
area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. vm-nnbd-sdk-complete All new NNBD and ported NNBD tests pass

Comments

@liamappelbe
Copy link
Contributor

I fixed some strong mode tests in https://dart-review.googlesource.com/c/sdk/+/141252, but it seems to have broken some dart2js and ddc tests:
https://logs.chromium.org/logs/dart/buildbucket/cr-buildbucket.appspot.com/8884361847891432912/+/steps/test_results/0/logs/new_test_failures__logs_/0
https://logs.chromium.org/logs/dart/buildbucket/cr-buildbucket.appspot.com/8884361135081414064/+/steps/test_results/0/logs/new_test_failures__logs_/0

I think the fixes are legit, so it's possible that these tests also need to be updated for strong mode.

@liamappelbe liamappelbe added area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. vm-nnbd-sdk-complete All new NNBD and ported NNBD tests pass labels Mar 31, 2020
@srujzs
Copy link
Contributor

srujzs commented Apr 2, 2020

For lib_2/html/element_test, it's relying on calling the List.length setter to throw an UnsupportedError => sdk_nnbd/lib/html/dart2js/html_dart2js.dart:11711 (GitHub isn't letting me view files this large so I can't directly link it). With the changes in that CL, it's never called. The proper fix is to probably implement insertAll in the class that extends ListBase and explicitly throw the error there.

@sigmundch
Copy link
Member

The ddc failure on error_stack_trace2_test is because we don't try to map the browser's stack overflow error to Dart's overflow error. It's pretty low-pri for us to do so, so I'm inclined to adjust how the test checks for this.

I sent you a CL here: https://dart-review.googlesource.com/c/sdk/+/142200

And file a bug for DDC here: #41308

dart-bot pushed a commit that referenced this issue Apr 2, 2020
Bug: #41275

insertAll should be unimplemented. It was previously relying
on a call to ChildrenElementList.length in the List mixin to
throw an UnsupportedError.

Change-Id: Ie8e5b126ea75a538e9db8b939bff0be012814efb
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/142201
Reviewed-by: Stephen Adams <sra@google.com>
Commit-Queue: Srujan Gaddam <srujzs@google.com>
@srujzs
Copy link
Contributor

srujzs commented Apr 2, 2020

lib_2/html/element_test should be passing now.

@sigmundch
Copy link
Member

Thanks, error_stack_trace2_test is passing since 42ce378, so I think we can close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. vm-nnbd-sdk-complete All new NNBD and ported NNBD tests pass
Projects
None yet
Development

No branches or pull requests

3 participants