Adds package:litetest, uses it instead of package:test under testing/dart #26215

zanderso · 2021-05-18T06:27:06Z

This PR adds a Dart testing library under testing/litetest. It is essentially a wrapper around package:async_helper from the Dart SDK source repo at //pkg/async_helper with modifications to make it work in the environment of flutter_tester. This wrapper is needed to ensure that all tests run to completion before the process exits. This is accomplished by opening a ReceivePort for each test, which is only closed when the test finishes running.

The API of package:litetest is pretty close to the parts of package:test used by the tests under testing/dart with some small differences requiring patch-ups that are included in this PR.

zanderso · 2021-05-19T16:08:16Z

testing/dart/isolate_name_server_test.dart

@@ -44,78 +44,82 @@ void isolateSpawnEntrypoint(SendPort port) {
 }

 void main() {
-  tearDown(() {


Replacing this tearDown with try {} finally {} in the tests.

zanderso · 2021-05-19T16:09:32Z

testing/dart/observatory/skp_test.dart

 import 'package:vm_service/vm_service.dart' as vms;
 import 'package:vm_service/vm_service_io.dart';

 void main() {
-  late vms.VmService vmService;
-
-  setUpAll(() async {


Replacing setUpAll and tearDownAll by inlining the code directly into the single test.

dnfield

Very cool! Only a couple real change requests (the instantiateImageCodec, adding a smoke test, and not using FutureOr). Otherwise nits.

dnfield · 2021-05-19T20:14:43Z

DEPS

+  'src/third_party/pkg/image':
+  Var('github_git') + '/brendan-duncan/image.git' + '@' + '3.0.2',
+
+  'src/third_party/pkg/petitparser':
+  Var('github_git') + '/petitparser/dart-petitparser' + '@' + '4.1.0',


These dependencies (and XML) could be painful to upgrade, which we'll eventually want to do. Can we just get rid of them?

It looks like we only use this to encode and write a PNG image. We should be able to do that via dart:ui.

(This, of course, should be done in a separate PR, and I'm not terribly partial to whether it's done before or after this one lands)

flutter/flutter#82950

Yeah, in this PR, I think I should leave the test logic as-is, but I am totally on board with cutting down on the dependencies.

testing/dart/channel_buffers_test.dart

testing/dart/codec_test.dart

dnfield · 2021-05-19T20:21:08Z

testing/dart/color_test.dart

    expect(const Color(0x12345678), equals(const Color(0x12345678)));
    expect(const Color(0x12345678), equals(Color(0x12345678))); // ignore: prefer_const_constructors
-    expect(const Color(0x12345678), isNot(equals(const Color(0x87654321))));
-    expect(const Color(0x12345678), isNot(equals(const NotAColor(0x12345678))));
-    expect(const NotAColor(0x12345678), isNot(equals(const Color(0x12345678))));
+    expect(const Color(0x12345678), notEquals(const Color(0x87654321)));
+    expect(const Color(0x12345678), notEquals(const NotAColor(0x12345678)));
+    expect(const NotAColor(0x12345678), notEquals(const Color(0x12345678)));
    expect(const NotAColor(0x12345678), equals(const NotAColor(0x12345678)));


FWIW, I'm not a fan of these kind of matchers anyway. I'd much rather just see expect(Color(...) == Color(...), true);, or even just make the second parameter to expect optional so that it more or less workslike assert.

That said, I like notEquals better than isNot(somethingElse(isNot(......

testing/litetest/lib/src/matchers.dart

testing/litetest/lib/src/test_suite.dart

testing/run_tests.py

dnfield · 2021-05-19T20:45:08Z

/cc @jonahwilliams @Hixie who will probably find this interesting.

…dart

jonahwilliams · 2021-05-20T04:11:34Z

Scandalous!

dnfield

LGTM

zanderso · 2021-05-20T06:08:48Z

Thanks for the thorough review!

…testing/dart (flutter/engine#26215)

…testing/dart (flutter/engine#26215) (#82992)

Hixie · 2021-05-20T20:57:33Z

If we're finding a way to get rid of test let's go ever further: drop test() (it doesn't actually add anything), group() (ditto), expect() (you can use assert), and so on. It would be so much simpler if a test was just straight dart code that either exit with a 0 or a 1 based on pass or fail...

zanderso · 2021-05-21T15:24:06Z

group() we can drop. test() has some advantages:

Each test can run in its own Zone without the unit test author worrying about Zones. This allows all tests to run even if one test has an unhandled asynchronous exception.
The test driver can control the scheduling of tests. For example it can run them in a random order, or limit the number of concurrently running tests.

Hixie · 2021-05-21T21:47:36Z

Those advantages could also be achieved by using different files for tests that need isolating from each other. That would have the advantage of actually isolating the tests, rather than only isolating some aspects of the tests (I frequently see issues where tests interfere with each other due to shared singletons, for example).

zanderso added the Work in progress (WIP) Not ready (yet) for review! label May 18, 2021

google-cla bot added the cla: yes label May 18, 2021

zanderso marked this pull request as draft May 18, 2021 06:27

zanderso force-pushed the dart-test-rm-test branch 3 times, most recently from ab5238c to abd7b11 Compare May 19, 2021 05:26

zanderso commented May 19, 2021

View reviewed changes

zanderso marked this pull request as ready for review May 19, 2021 16:11

zanderso removed the Work in progress (WIP) Not ready (yet) for review! label May 19, 2021

zanderso requested a review from dnfield May 19, 2021 16:11

zanderso mentioned this pull request May 19, 2021

Removing testing/smoke_test_failure #26130

Closed

dnfield suggested changes May 19, 2021

View reviewed changes

zanderso force-pushed the dart-test-rm-test branch 2 times, most recently from 1371d68 to f38a8cd Compare May 20, 2021 03:14

Adds package:litetest, uses it instead of package:test under testing/…

9ffb882

…dart

zanderso force-pushed the dart-test-rm-test branch from f38a8cd to 9ffb882 Compare May 20, 2021 04:16

dnfield approved these changes May 20, 2021

View reviewed changes

zanderso merged commit dd24949 into flutter:master May 20, 2021

zanderso deleted the dart-test-rm-test branch May 20, 2021 06:08

engine-flutter-autoroll mentioned this pull request May 20, 2021

Roll Engine from 67c537d41bab to dd24949ca143 (1 revision) flutter/flutter#82992

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 20, 2021

dd24949 Adds package:litetest, uses it instead of package:test under …

df3a28b

…testing/dart (flutter/engine#26215)

fluttergithubbot pushed a commit to flutter/flutter that referenced this pull request May 20, 2021

dd24949 Adds package:litetest, uses it instead of package:test under …

990cc6d

…testing/dart (flutter/engine#26215) (#82992)

zanderso mentioned this pull request May 20, 2021

Add Dart SPIR-V transpiler #25480

Merged

9 tasks

zanderso mentioned this pull request May 22, 2021

Remove group() support from package:litetest #26342

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds package:litetest, uses it instead of package:test under testing/dart #26215

Adds package:litetest, uses it instead of package:test under testing/dart #26215

zanderso commented May 18, 2021 •

edited

Loading

zanderso May 19, 2021

zanderso May 19, 2021

dnfield left a comment

dnfield May 19, 2021

dnfield May 19, 2021

dnfield May 19, 2021

dnfield May 19, 2021

zanderso May 19, 2021

dnfield May 19, 2021

dnfield commented May 19, 2021

jonahwilliams commented May 20, 2021

dnfield left a comment

zanderso commented May 20, 2021

Hixie commented May 20, 2021

zanderso commented May 21, 2021

Hixie commented May 21, 2021

Adds package:litetest, uses it instead of package:test under testing/dart #26215

Adds package:litetest, uses it instead of package:test under testing/dart #26215

Conversation

zanderso commented May 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnfield left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnfield commented May 19, 2021

jonahwilliams commented May 20, 2021

dnfield left a comment

Choose a reason for hiding this comment

zanderso commented May 20, 2021

Hixie commented May 20, 2021

zanderso commented May 21, 2021

Hixie commented May 21, 2021

zanderso commented May 18, 2021 •

edited

Loading