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

Handle the new formatting style in the static error test updater #57042

Closed
munificent opened this issue Nov 6, 2024 · 1 comment
Closed

Handle the new formatting style in the static error test updater #57042

munificent opened this issue Nov 6, 2024 · 1 comment
Labels
area-test Cross-cutting test issues (use area- labels for specific failures; not used for package:test).

Comments

@munificent
Copy link
Member

We have an automated tool in pkg/test_runner/tool/update_static_error_tests.dart that will automatically regenerate the test expectation comments in static error tests. That tool generally tries to indent the expectation comments at the same level as the previous line, like:

class C {
  void method() {
    some + code;
    //   ^ Some error expectation on "+".
  }
}

But if the error marker is too early in the line, it will put the comment at column zero. This was safe to do because the old formatter never indented a line comment if it was at the beginning of the line. That feature is handy here, but was basically an anti-feature everywhere else, so the new formatter doesn't do that.

That means that if you update the test expectations and then format it, expectation comments may get shifted over and point at the wrong column. We should update update_static_error_tests.dart so that it always indents the comment with the previous line and then writes an explicit column number (which the static error test already supports) if the column needs to be before where the indented // starts.

@munificent munificent added the area-test Cross-cutting test issues (use area- labels for specific failures; not used for package:test). label Nov 6, 2024
@lrhn
Copy link
Member

lrhn commented Nov 13, 2024

How do we know the start of the previous line?

If I have:

  var qux = 
      fooLongName() +
          barLongName();
  // some comment about the next line, or the state after the prior line.
  baz();

the comment indentation should be that of the var qux = line. (Right?)

If I am correct, I think it means the update_static_error_test.dart file needs to be able to do some amount of expression and statement parsing to know where the prior statement started. It can't just put it relative to that
last line.
(Or do we just assume that tests are simple and the statement with the error is on one line? Which might very well be true.)

copybara-service bot pushed a commit that referenced this issue Nov 13, 2024
I'm slowly going through and reformatting the tests. That also means
regenerating the static error expectations because of #57042. But I
don't need to generate static error markers for files that don't already
have them.

So this adds a flag to the test updater to let it skip over any test
file that isn't already known to be a static error test.

Change-Id: I61f10d29924f1f9d2dabf61bb604e53a6622017f
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/394940
Auto-Submit: Bob Nystrom <[email protected]>
Reviewed-by: Nicholas Shahan <[email protected]>
Commit-Queue: Bob Nystrom <[email protected]>
copybara-service bot pushed a commit that referenced this issue Nov 14, 2024
I also regenerated the static error expectations (see #57042 for
context).

Change-Id: Ia0ca22604a0dd422cc265a46845afc43f551fe38
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/394572
Commit-Queue: Lasse Nielsen <[email protected]>
Reviewed-by: Lasse Nielsen <[email protected]>
Auto-Submit: Bob Nystrom <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-test Cross-cutting test issues (use area- labels for specific failures; not used for package:test).
Projects
None yet
Development

No branches or pull requests

2 participants