Smarter versioned tests (#1744)

Redo how versioned tests work.

The diff in the README.md and the doc comments in the .dart file changes should explain what's going on here. The basic idea is that now a test is either completely unversioned with only a single output, or completely versioned where all of the output sections have a version number.

I think that's easier to read than a test like:

```
<<<
some.code();
>>> 3.8
some.code();
>>>
some.code();
```

And you have to try to figure out what versions the last section applies to.

This commit also updates the tests to match the new style. This just means adding `3.8` to a bunch of output sections where that was previously implicit. The set of tested versions and their expectations are the same.

I also updated the test updater to support the new output styles.

This change also speeds up running the formatter tests by skipping running tests on language versions where the output is the same as a previous and subsequent version. Prior to this change, running tall_format_test.dart runs 8,674 tests. If you bump the highest supported language version from 3.9 to 3.10 (which I'm about to do for dot shorthands), that jumps to 11,556 because it runs every test at every language version.

With this change, it runs 5,940 tests when the maximum supported version is both 3.9 and 3.10. The number of tests being run only increases when there are actual new tests or new version-specific expectations.

Author: munificent

example/format.dart

@@ -116,8 +116,12 @@ Future<void> _runTest(
   var actualText = actual.textWithSelectionMarkers;
   if (!testFile.isCompilationUnit) actualText += '\n';
 
-  // TODO(rnystrom): Handle multiple outputs.
-  var expectedText = formatTest.outputs.first.code.textWithSelectionMarkers;
+  var output = switch (formatTest) {
+    UnversionedFormatTest(:var output) => output,
+    // Used the newest style for the expectation.
+    VersionedFormatTest(:var outputs) => outputs.values.last,
+  };
+  var expectedText = output.code.textWithSelectionMarkers;
 
   print('$path ${formatTest.input.description}');
   _drawRuler('before', pageWidth);

lib/src/testing/test_file.dart

@@ -115,6 +115,7 @@ final class TestFile {
       var lineNumber = i + 1;
       var line = readLine().replaceAll('>>>', '');
       var (options, description) = _parseOptions(line);
+      description = description.trim();
 
       var inputComments = readComments();
       var inputBuffer = StringBuffer();
@@ -127,9 +128,27 @@ final class TestFile {
         isCompilationUnit: isCompilationUnit,
       );
 
-      var input = TestEntry(description.trim(), null, inputComments, inputCode);
+      var input = TestEntry(description, inputComments, inputCode);
+
+      // Read the outputs. A single test should have outputs in one of two
+      // forms:
+      //
+      // - One single unversioned output which is the expected output across
+      //   all supported versions.
+      // - One or more versioned outputs, each of which defines the expected
+      //   output at that language version or later until reaching the next
+      //   output's version.
+      //
+      // The parser here collects all of the outputs, versioned and unversioned
+      // and then reports an error if the result is not one of those two styles.
+      void fail(String error) {
+        throw FormatException(
+          'Test format error in $relativePath, line $lineNumber: $error',
+        );
+      }
 
-      var outputs = <TestEntry>[];
+      var unversionedOutputs = <TestEntry>[];
+      var versionedOutputs = <Version, TestEntry>{};
       while (i < lines.length && lines[i].startsWith('<<<')) {
         var match = _outputPattern.firstMatch(readLine())!;
         var outputDescription = match[4]!;
@@ -166,17 +185,43 @@ final class TestFile {
           isCompilationUnit: isCompilationUnit,
         );
 
-        outputs.add(
-          TestEntry(
-            outputDescription.trim(),
-            outputVersion,
-            outputComments,
-            outputCode,
-          ),
+        var entry = TestEntry(
+          outputDescription.trim(),
+          outputComments,
+          outputCode,
         );
+        if (outputVersion != null) {
+          if (versionedOutputs.containsKey(outputVersion)) {
+            fail('Multiple outputs with the same version $outputVersion.');
+          }
+
+          versionedOutputs[outputVersion] = entry;
+        } else {
+          unversionedOutputs.add(entry);
+        }
       }
 
-      tests.add(FormatTest(lineNumber, options, input, outputs));
+      switch ((unversionedOutputs.length, versionedOutputs.length)) {
+        case (0, 0):
+          fail('Test must have at least one output.');
+        case (0, > 0):
+          tests.add(
+            VersionedFormatTest(lineNumber, options, input, versionedOutputs),
+          );
+        case (1, 0):
+          tests.add(
+            UnversionedFormatTest(
+              lineNumber,
+              options,
+              input,
+              unversionedOutputs.first,
+            ),
+          );
+        case (> 1, 0):
+          fail('Test can\'t have multiple unversioned outputs.');
+        default:
+          fail('Test can\'t have both versioned and unversioned outputs.');
+      }
     }
 
     return TestFile._(
@@ -276,7 +321,7 @@ final class TestFile {
 }
 
 /// A single formatting test inside a [TestFile].
-final class FormatTest {
+sealed class FormatTest {
   /// The 1-based index of the line where this test begins.
   final int line;
 
@@ -286,13 +331,7 @@ final class FormatTest {
   /// The unformatted input.
   final TestEntry input;
 
-  // TODO(rnystrom): Consider making this a map of version (or null) to output
-  // and then validating that there aren't duplicate outputs for a single
-  // version.
-  /// The expected output.
-  final List<TestEntry> outputs;
-
-  FormatTest(this.line, this.options, this.input, this.outputs);
+  FormatTest(this.line, this.options, this.input);
 
   /// The line and description of the test.
   String get label {
@@ -301,20 +340,45 @@ final class FormatTest {
   }
 }
 
+/// A test for formatting that should be the same across all language versions.
+///
+/// Most tests are of this form.
+final class UnversionedFormatTest extends FormatTest {
+  /// The expected output.
+  final TestEntry output;
+
+  UnversionedFormatTest(super.line, super.options, super.input, this.output);
+}
+
+/// A test whose expected formatting changes at specific versions.
+final class VersionedFormatTest extends FormatTest {
+  /// The expected output by version.
+  ///
+  /// Each key is the lowest version where that output is expected. If there are
+  /// supported versions lower than the lowest key here, then the test is not
+  /// run on those versions at all. These tests represent new syntax that isn't
+  /// supported in later versions. For example, if the map has only a single
+  /// entry whose key is 3.8, then the test is skipped on 3.7, run at 3.8, and
+  /// should be valid at any higher version.
+  ///
+  /// If there are multiple entries in the map, they represent versions where
+  /// the formatting style has changed.
+  final Map<Version, TestEntry> outputs;
+
+  VersionedFormatTest(super.line, super.options, super.input, this.outputs);
+}
+
 /// A single test input or output.
 final class TestEntry {
   /// Any remark on the "<<<" or ">>>" line.
   final String description;
 
-  /// If this is a test output for a specific version, the version.
-  final Version? version;
-
   /// The `###` comment lines appearing after the header line before the code.
   final List<String> comments;
 
   final SourceCode code;
 
-  TestEntry(this.description, this.version, this.comments, this.code);
+  TestEntry(this.description, this.comments, this.code);
 }
 
 /// Options for configuring all tests in a file or an individual test.

test/README.md

@@ -37,15 +37,57 @@ description for the test. Lines after that define the input code to be
 formatted.
 
 After the input are one or more output sections. Each output section starts
-with a header like:
+with a header that starts with `<<<`. There are two styles of output:
+
+#### Unversioned output
+
+Most code is supported across all language versions and formats the same way in
+all of them. For those, the output is a single section like:
+
+```
+>>> Optional input description.
+some.code();
+<<< Optional description.
+some.code();
+```
+
+The formatter will run this tests against the oldest and newest supported
+version and verify that both produce that output. (We assume that if it formats
+the same at two versions, it will do so in any version between those for
+performance reasons. Otherwise, every time a new Dart SDK release comes out,
+the number of tests being run increases by thousands.)
+
+#### Versioned outputs
+
+The formatter's behavior may depend on language version for two reasons:
+
+* New syntax was added to the language in a later version, so we can't format
+  it on an older version at all.
+
+* The formatting style changed and we versioned the style change so that code
+  at older versions keeps the older style.
+
+To accommodate those, a test can have multiple output sections which each start
+with a version number like this:
 
 ```
-<<< 3.7 Optional description.
+>>> Optional input description.
+some.code();
+<<< 3.8 Optional description.
+some.code();
+<<< 3.10 Optional description.
+some . code();
 ```
 
-The `<<<` marks the beginning of a new output section. If it has a language
-version number, then this output is expected only on that language version. If
-it has no version number, then this is the expected output on all versions.
+Each output section specifies the minimum version where that output becomes
+expected. The version number of the first section specifies the lowest version
+number that the test will be run at. Every section after that specifies a
+version where the formatting style was changed.
+
+The test is run at multiple language versions and the result compared to the
+appropriate output section for that version. In the example here, we won't test
+it at all at 3.7, will test at 3.8 and 3.9 with the first output, and at
+3.10 and higher with the last output.
 
 ### Test options
 

test/tall/declaration/typedef.unit

@@ -160,26 +160,26 @@ typedef Generic<T, R> =
     );
 >>> Allow block-formatting a record typedef.
 typedef SomeType = (int first, int second);
-<<<
-typedef SomeType = (
-  int first,
-  int second,
-);
 <<< 3.7
 typedef SomeType =
     (int first, int second);
->>> Don't allow block-formatting a record typedef.
-typedef SomeType = (int first, int second, String third);
-<<<
+<<< 3.8
 typedef SomeType = (
   int first,
   int second,
-  String third,
 );
+>>> Don't allow block-formatting a record typedef.
+typedef SomeType = (int first, int second, String third);
 <<< 3.7
 typedef SomeType =
     (
       int first,
       int second,
       String third,
     );
+<<< 3.8
+typedef SomeType = (
+  int first,
+  int second,
+  String third,
+);

test/tall/expression/assignment.stmt

@@ -81,13 +81,6 @@ target
     reallyLongValue;
 >>> Allow block formatting through nested assignments.
 outer = inner = [element1, element2, element3, element4];
-<<<
-outer = inner = [
-  element1,
-  element2,
-  element3,
-  element4,
-];
 <<< 3.7
 outer =
     inner = [
@@ -96,19 +89,26 @@ outer =
       element3,
       element4,
     ];
+<<< 3.8
+outer = inner = [
+  element1,
+  element2,
+  element3,
+  element4,
+];
 >>> Headline format unsplit target of call chain.
 variable = (tar + get).method().another().third();
-<<<
-variable = (tar + get)
-    .method()
-    .another()
-    .third();
 <<< 3.7
 variable =
     (tar + get)
         .method()
         .another()
         .third();
+<<< 3.8
+variable = (tar + get)
+    .method()
+    .another()
+    .third();
 >>> Don't headline format target of call chain if target splits.
 variable = (veryLongTarget + expressionThatSplits).method().another().third();
 <<<
@@ -120,14 +120,14 @@ variable =
         .third();
 >>> Headline format unsplit properties of call chain.
 variable = (tar + get).prop.erty.method().another().third();
-<<<
-variable = (tar + get).prop.erty
-    .method()
-    .another()
-    .third();
 <<< 3.7
 variable =
     (tar + get).prop.erty
         .method()
         .another()
         .third();
+<<< 3.8
+variable = (tar + get).prop.erty
+    .method()
+    .another()
+    .third();