Add ShouldGenerateFile annotation #32

alexeyinkin · 2022-09-18T08:08:23Z

This PR adds ShouldGenerateFile annotation to expect the match of the generated code against the content of an existing file.

Resolves Test if the generated code matches a file #31

Rationale

For an example use, see this package: https://github.com/alexeyinkin/dart-enum-map/tree/main/enum_map_gen (made to use a fork of source_gen_test for now)

The package generates code of 160+ lines, so to verify the output it is useful to not only test that the output matches a string, but also to write tests on that output itself.

This file uses the new ShouldGenerateFile annotation:
https://github.com/alexeyinkin/dart-enum-map/blob/main/enum_map_gen/test/unmodifiable_enum_map/src/input.dart

This is the golden that the output is compared with:
https://github.com/alexeyinkin/dart-enum-map/blob/main/enum_map_gen/test/unmodifiable_enum_map/src/output.dart

This is the test on that golden:
https://github.com/alexeyinkin/dart-enum-map/blob/main/enum_map_gen/test/unmodifiable_enum_map/unmodifiable_enum_map_test.dart

This pair of tests ensures that the output both matches the desired one and works as expected.

Implementation

The new annotation has optional arguments partOf and partOfCurrent.

partOfCurrent adds to the output file a link back to the input file, so that the input file can have tests written for it, as in the example above.

partOf adds to the output file a link to an arbitrary file, if for some reason tests cannot be written for the input file. I can imagine cases where the input file is suitable for LibraryReader but not for actual execution, so this may be useful. Although if anyone really wants tests on their output they better refactor their input to be buildable. If in doubt, this functionality can be removed.

If both arguments are omitted, the output file is just the generated content with no additions. This is useful if no tests are planned on the output but it is just too large to be inlined in the input file.

Open Questions

1. Definition of 'golden'

In the wild, a 'golden' test seems to mean comparison to a known result, most often in separate files. This sense of 'golden' is used in Flutter with its --update-goldens flag (it is not called --update-golden-files). But current ShouldGenerate may also be said to compare with a golden that is just being an inline string. Is it OK to name the new annotation ShouldGenerateGolden or should it be ShouldGenerateFile or anything else?

The annotation renamed to ShouldGenerateFile.

2. Updating golden files

There is a need to automatically generate golden files. Flutter does this with --update-goldens flag. If it is set, then the output is not tested against goldens, but they are created or updated with a test's output.

The pure Dart does not have this concept, so we cannot use this flag to decide if we need to compare or update goldens.

Ideally we want Dart to also have a concept of golden files and to have a similar flag. Then we could have tapped into it and compare or update accordingly. The concept of golden tests is actually wider than what Flutter assumes (in Flutter it only means screenshot matches), so making Dart aware of golden files feels natural. I believe there are other applications for this in Dart like testing JSON serializers with large output, etc. Maybe we can file a feature request for this in Dart SDK repo?

For now, to generate the output I introduced the support for SOURCE_GEN_TEST_UPDATE_GOLDENS environment variable. If it is set to '1', the goldens are updated. This seems to be the easiest way to tell the comparison from the update without touching the code.

Is this OK or should we do something else to update the files?

README.md

alexeyinkin · 2022-10-12T04:54:37Z

@kevmoo can you please share your view of this change?

I could have done this generator in a separate package that would depend on this one, but I cannot customize the private _mappers with a new annotation unless it is in this package.

kevmoo · 2022-10-12T05:27:04Z

Ooo!

kevmoo · 2022-10-12T05:34:43Z

Need some formatter help!

alexeyinkin · 2022-10-12T05:42:38Z

Fixed.

# Conflicts: # analysis_options.yaml

alexeyinkin · 2023-01-21T09:17:38Z

@kevmoo can you please take a look again? I would really like to have this in an established package like yours instead of having to make another package for such tests. Not to mention that this is likely a common need.

alexeyinkin · 2023-01-22T03:27:23Z

@kevmoo pardon me, all fixed. Same output as on master:

alexey@alexey-akvelon-dell:~/work/dart/source_gen_test$ dart analyze
Analyzing source_gen_test...           1.5s
No issues found!
alexey@alexey-akvelon-dell:~/work/dart/source_gen_test$ dart format -o none .
Formatted 24 files (0 changed) in 0.37 seconds.
alexey@alexey-akvelon-dell:~/work/dart/source_gen_test$ dart test
00:02 +3: test/example_test.dart: BadTestClass with configuration "no-prefix-required"
  NOTE: Could not find an annotation that matched
      asset:source_gen_test/example/src/example_annotation.dart#ExampleAnnotation.
    Using a annotation with the same name from the synthetic library instead
      package:__test__/example_annotation.dart#ExampleAnnotation
00:02 +17: test/generate_for_element_test.dart: generateForElement TestClass1  
  NOTE: Could not find an annotation that matched
      asset:source_gen_test/test/src/test_annotation.dart#TestAnnotation.
    Using a annotation with the same name from the synthetic library instead
      package:__test__/test_annotation.dart#TestAnnotation
00:02 +57: All tests passed!

alexeyinkin · 2023-06-25T07:52:00Z

@kevmoo can you please take a look once again?

This time I ran your CI on my fork, and it's green: alexeyinkin#3

This PR also adds trailing commas that broke your last run:
https://github.com/kevmoo/source_gen_test/actions/runs/5366912806/jobs/9736633364

btrautmann · 2023-06-28T19:50:32Z

Would love to see this land! I was actually just working on a PR for this very same thing and decided to see if somebody was already working on it.

I also +1 the idea of being able to signal to re-generate goldens! I do wonder though, with the environment variable, is there a way to specify only wanting to re-generate select goldens? I.e if I change some functionality and only expect 20/100 test cases to change, how can I generate only those ones? My original plan was to go with another bit passed to the ShouldGenerateFile constructor which would allow for this flexibility with the obvious overhead of having to add/remove these bits manually. Maybe supporting that, in addition to the environment variable would suffice?

One caveat about the non-environment-variable approach: I think doing it this way should fail the test. This is similar to how (IIRC) Android Lint fails the build when generating a new lint_baseline because doing so may be unintentional (the signal to do so is a flag set within build.gradle which may be accidentally left enabled). I think the same thing should happen here, where after generating them, the test is failed with a message indicating that goldens have been regenerated and the bit should be removed.

alexeyinkin · 2023-06-29T06:09:26Z

The best way to update only specific goldens is to run the specific tests:

SOURCE_GEN_TEST_UPDATE_GOLDENS=1 dart test my_test.dart
SOURCE_GEN_TEST_UPDATE_GOLDENS=1 dart test -n "my test name"

With this feature at hand, any userland awareness of this is redundant.
I also want this to be consistent with flutter test --update-goldens ..., which has no way to specify which goldens to update, and this simplicity is good.

# Conflicts: # CHANGELOG.md # pubspec.yaml

alexeyinkin · 2023-07-12T12:27:59Z

@btrautmann while this is being reviewed, you can use a temporary package I created:
https://pub.dev/packages/source_gen_test_golden

I use it in my own production packages. I intend to deprecate it if @kevmoo merges this.

Use that package if you are developing a package of your own and you cannot afford git dependencies. Otherwise, if you are making an app, you can use the git dependency of my branch:
https://github.com/alexeyinkin/source_gen_test/tree/issue31_golden

Use the hash of the latest commit from there.

kevmoo · 2023-07-12T17:24:10Z

Will try to get to this today!

btrautmann · 2023-07-12T20:28:37Z

Just wanna say thanks to @alexeyinkin... I've been using the package he published while this PR awaits merge, and it's made my life 100000x easier. Looking forward to this merging! Thanks again @alexeyinkin and @kevmoo!

kevmoo · 2023-07-12T22:48:56Z

When I run this locally I see

00:01 +3: test/example_test.dart: BadTestClass with configuration "no-prefix-required"
  NOTE: Could not find an annotation that matched
      asset:source_gen_test/example/src/example_annotation.dart#ExampleAnnotation.
    Using a annotation with the same name from the synthetic library instead
      package:__test__/example_annotation.dart#ExampleAnnotation
00:01 +19: test/generate_for_element_test.dart: generateForElement TestClass1
  NOTE: Could not find an annotation that matched
      asset:source_gen_test/test/src/test_annotation.dart#TestAnnotation.
    Using a annotation with the same name from the synthetic library instead
      package:__test__/test_annotation.dart#TestAnnotation

🤔

alexeyinkin · 2023-07-12T23:10:23Z

I have this even on master, so I did not investigate:

alexey@alexey-X550CC:~/work/dart/source_gen_test$ git log -1
commit d31503b22c2595684fca22149df2c264869d0985 (HEAD -> master, origin/master, origin/HEAD)
Author: Kevin Moore <kevmoo@users.noreply.github.com>
Date:   Wed Jul 12 14:36:55 2023 -0700

    blast_repo fixes (#53)
    
    auto-publish, dependabot, no-response
alexey@alexey-X550CC:~/work/dart/source_gen_test$ dart pub get
Resolving dependencies... (1.2s)
  _fe_analyzer_shared 61.0.0 (62.0.0 available)
  analyzer 5.13.0 (6.0.0 available)
  build 2.4.0 (2.4.1 available)
  build_resolvers 2.2.0 (2.2.1 available)
  build_test 2.1.7 (2.2.0 available)
+ dart_flutter_team_lints 1.0.0
  dart_style 2.3.1 (2.3.2 available)
  source_gen 1.3.2 (1.4.0 available)
  stack_trace 1.11.0 (1.11.1 available)
  test 1.24.3 (1.24.4 available)
  test_api 0.6.0 (0.6.1 available)
  test_core 0.5.3 (0.5.4 available)
Changed 1 dependency!
alexey@alexey-X550CC:~/work/dart/source_gen_test$ dart test
Building package executable... (10.9s)
Built test:test.
00:12 +4: test/example_test.dart: BadTestClass with configuration "no-prefix-required"                                                       
  NOTE: Could not find an annotation that matched
      asset:source_gen_test/example/src/example_annotation.dart#ExampleAnnotation.
    Using a annotation with the same name from the synthetic library instead
      package:__test__/example_annotation.dart#ExampleAnnotation
00:14 +17: test/generate_for_element_test.dart: generateForElement TestClass1                                                                
  NOTE: Could not find an annotation that matched
      asset:source_gen_test/test/src/test_annotation.dart#TestAnnotation.
    Using a annotation with the same name from the synthetic library instead
      package:__test__/test_annotation.dart#TestAnnotation
00:14 +48: All tests passed!

So this is not a regression.

kevmoo · 2023-07-13T01:40:59Z

Ack @alexeyinkin, re the print statements

One thought:

I think tests should FAIL w/ the environment variable set. It's trivial to re-run them. I like the idea that the goldens are wired up to regenerate on CI accidentally or something. I've done similar things in other packages – thoughts?

alexeyinkin · 2023-07-13T02:05:09Z

I also like the idea that updating goldens should fail the test. It is safer. However, long-term we should make things consistent across Dart and Flutter. In Flutter, --update-goldens currently results in tests pass successfully. I think, this should be changed, and this is a breaking change for Flutter or flutter_test.

I suggest filing an issue for that in Flutter for a wider discussion, and then make this PR consistent with the result of that discussion.

alexeyinkin added 2 commits September 18, 2022 10:22

Add ShouldGenerateGolden

3ccd52b

Hint to update golden files on failures (kevmoo#31)

3c76e3c

alexeyinkin commented Sep 18, 2022

View reviewed changes

README.md Show resolved Hide resolved

Auto-format (kevmoo#31)

3a662ea

alexeyinkin added 2 commits January 21, 2023 12:26

Merge branch 'master' into issue31_golden

4e2f155

# Conflicts: # analysis_options.yaml

Rename *Golden to *File (kevmoo#31)

e0e5959

alexeyinkin changed the title ~~Add ShouldGenerateGolden annotation~~ Add ShouldGenerateFile annotation Jan 21, 2023

Fix lints (kevmoo#31)

8ceebc3

alexeyinkin added 2 commits June 25, 2023 10:51

Merge branch 'master' into issue31_golden

629c6e6

Add trailing commas (kevmoo#31)

34f2f6e

Update CHANGELOG (kevmoo#31)

bf99429

alexeyinkin mentioned this pull request Jul 5, 2023

Bring --update-goldens from Flutter to pure Dart dart-lang/test#2057

Open

alexeyinkin added 2 commits July 2, 2023 06:40

Add ShouldGenerateFile to example (kevmoo#31)

375b552

Merge branch 'master' into issue31_golden

1ac6e63

# Conflicts: # CHANGELOG.md # pubspec.yaml

kevmoo added 2 commits July 12, 2023 14:33

New feature should bump minor version

7785600

Merge remote-tracking branch 'origin/master' into issue31_golden

2e4e432

alexeyinkin mentioned this pull request Jul 13, 2023

--update-goldens should fail the tests flutter/flutter#130489

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ShouldGenerateFile annotation #32

Add ShouldGenerateFile annotation #32

alexeyinkin commented Sep 18, 2022 •

edited

alexeyinkin commented Oct 12, 2022

kevmoo commented Oct 12, 2022

kevmoo commented Oct 12, 2022

alexeyinkin commented Oct 12, 2022

alexeyinkin commented Jan 21, 2023

alexeyinkin commented Jan 22, 2023

alexeyinkin commented Jun 25, 2023

btrautmann commented Jun 28, 2023

alexeyinkin commented Jun 29, 2023

alexeyinkin commented Jul 12, 2023

kevmoo commented Jul 12, 2023

btrautmann commented Jul 12, 2023

kevmoo commented Jul 12, 2023

alexeyinkin commented Jul 12, 2023

kevmoo commented Jul 13, 2023

alexeyinkin commented Jul 13, 2023

Add ShouldGenerateFile annotation #32

Are you sure you want to change the base?

Add ShouldGenerateFile annotation #32

Conversation

alexeyinkin commented Sep 18, 2022 • edited

Rationale

Implementation

Open Questions

1. Definition of 'golden'

2. Updating golden files

alexeyinkin commented Oct 12, 2022

kevmoo commented Oct 12, 2022

kevmoo commented Oct 12, 2022

alexeyinkin commented Oct 12, 2022

alexeyinkin commented Jan 21, 2023

alexeyinkin commented Jan 22, 2023

alexeyinkin commented Jun 25, 2023

btrautmann commented Jun 28, 2023

alexeyinkin commented Jun 29, 2023

alexeyinkin commented Jul 12, 2023

kevmoo commented Jul 12, 2023

btrautmann commented Jul 12, 2023

kevmoo commented Jul 12, 2023

alexeyinkin commented Jul 12, 2023

kevmoo commented Jul 13, 2023

alexeyinkin commented Jul 13, 2023

alexeyinkin commented Sep 18, 2022 •

edited