Sdk: Add static error tests for DDC and dart2js

[natebosch]

https://dart-review.googlesource.com/c/sdk/+/130733 adds some static checks that apply only in DDC and Dart2JS for now. We need to add tests for these, but the current infrastructure expects to run tests for static errors with front end and analyzer only.

@munificent - do you have any ideas on the best approach for this?

[natebosch]

Do these tests still work?

https://github.com/dart-lang/sdk/blob/15b6f19008e34e5cdcfb555ac899d29dcbd0f1f1/tests/compiler/dart2js_extra/non_jsinterop_test.dart#L23

cc @sigmundch

[natebosch]

cc @johnniwinther - I see a note about sharing these tests with DDC:

https://github.com/dart-lang/sdk/blob/15b6f19008e34e5cdcfb555ac899d29dcbd0f1f1/tests/compiler/dart2js_extra/non_jsinterop_test.dart#L5

Do you have any background you can share on these tests? How do I run them? How would I update these if I fix some of the missing compile time errors? Is it enough to remove some lines from the legacy_status_dart2js.csv file? https://github.com/dart-lang/sdk/blob/d99f6bffcb11f21def4cee7032039512b220684c/tests/legacy_status_dart2js.csv#L37

What do you expect it would take to have use this test for DDC?

[johnniwinther]

tests/compiler/dart2js_extra/non_jsinterop_test.dart and it's sibling
tests/compiler/dart2js_extra/jsinterop_test.dart can be as multitests with
test.py:

python ./tools/test.py --arch=x64 -mrelease -cdart2js -rd8
dart2js_extra/jsinterop

This tests that error case behaves as expected - which it current doesn't.
This way of running the tests should be fairly easy to use for DDC since it
only relies on an error being reported or not.

The expectations in these tests (in comments like //
JS_INTEROP_FIELD_NOT_SUPPORTED ) are tested with the dart2js unittest:

dart tests/compiler/dart2js/model/native_test.dart

This tests that dart2js reports the expected error for the error cases, as
well as reasons as expected about which classes and members are
native/jsinterop.
This test is hard to use for DDC since it relies on the exact kinds of
errors reported and these error kinds are currently internal to dart2js.

When reporting some more errors should remove the corresponding line
numbers mentioned in the 'skipList' entries in
tests/compiler/dart2js/model/native_test.dart to verify that dart2js now
emits the expected error.
I didn't know about tests/legacy_status_dart2js.csv so I don't know whether
it is necessary to remove the lines here, but it seems that we should.

On Fri, 28 Feb 2020 at 02:02, Nate Bosch [e-mail] wrote:

cc @johnniwinther https://github.com/johnniwinther - I see a note about
sharing these tests with DDC:

https://github.com/dart-lang/sdk/blob/15b6f19008e34e5cdcfb555ac899d29dcbd0f1f1/tests/compiler/dart2js_extra/non_jsinterop_test.dart#L5

Do you have any background you can share on these tests? How do I run
them? How would I update these if I fix some of the missing compile time
errors? Is it enough to remove some lines from the
legacy_status_dart2js.csv file?
https://github.com/dart-lang/sdk/blob/d99f6bffcb11f21def4cee7032039512b220684c/tests/legacy_status_dart2js.csv#L37

What do you expect it would take to have use this test for DDC?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/dart-lang/sdk/issues/40360?email_source=notifications&email_token=AC6ZYJKWWD6ICQD4EYEBVYLRFBPBBA5CNFSM4KM2G3V2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENGR5WQ#issuecomment-592256730,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AC6ZYJMGQQCY4O5DR3DDRRDRFBPBBANCNFSM4KM2G3VQ
.

[sigmundch]

I didn't know about tests/legacy_status_dart2js.csv so I don't know whether
it is necessary to remove the lines here, but it seems that we should.

Deleting the relevant entries sounds good. My understanding is that Karl added this file to track the comments that long ago were written in .status files. It was added to make it possible to delete the extra entries in the status files without loosing the information that linked test failures and github issues. Besides being data, I believe the .csv file has no other purpose and is not involved in any test execution/status.

[natebosch]

Looks like there are more similar tests at https://github.com/dart-lang/sdk/blob/1fe6ca96d0665fe57af0d14a156aa97b361f480c/tests/compiler/dart2js/jsinterop/declaration_test.dart

[munificent]

Off the top of my head, one option would be to add a new requirement like:

// Requirements=web-compiler

And then add a little special logic to the test runner to only run static error tests if:

1. The configuration is front end or analyzer.
2. Or the configuration is DDC or dart2js and this requirement is present.

That would cause these tests to not get skipped while also avoiding running all of other the static error tests on DDC and dart2js.

It feels a little hacky, though. I was hoping the limited Boolean logic supported by the requirements stuff would be sufficient but it seems we keep running into cases like this where we want to express something more complex. Maybe we should do actual Boolean expression support like the status files support for matching test groups, but place it in the test file itself.

[munificent]

Here's a better idea now that I've put a little brain juice into it:

In the test runner, we use the specific static error test markers to determine which platforms to run a static error test on. So if it contains a [cfe] marker, then we run it on the CFE. [analyzer] gets it run on analyzer. On any other platforms, the test is skipped. This mirrors the behavior we see today since every static error test does contain both [cfe] and [analyzer] markers.

Then we add a third marker like [web] for errors that should be reported by DDC and dart2js. Any test containing one of those markers is run by DDC and dart2js as a static error test. If the test only contains that marker, then the other platforms will implicitly skip it.

How does that sound?

[sigmundch]

love it!

[munificent]

This is all done now.