Skip to content

Conversation

@kuhnroyal
Copy link
Contributor

@kuhnroyal kuhnroyal commented Aug 3, 2021

  • Fixes problem described in Mocking a moor database fails #459 around factory redirects into different source files/imports
  • add tests
  • gitignore build folder and generated mock test files

Fixes #459

@google-cla google-cla bot added the cla: yes label Aug 3, 2021
@srawlins
Copy link
Member

srawlins commented Aug 4, 2021

I like the idea here, but I think that the parameter element is not sufficient to choosing the right element to reference. For example, here is a test (that you can insert into auto_mocks_test.dart) which does not pass with this fix:

  test(
      'matches parameter default values constructed from a const factory '
      'constructor2', () async {
    var mocksContent = await buildWithNonNullable({
      ...annotationsAsset,
      ...simpleTestAsset,
      'foo|lib/foo.dart': '''
      import 'baz.dart';
      class Foo {
        void m([Bar a = const Baz.named()]) {}
      }
      class Bar {
        const Bar();
      }
      ''',
      'foo|lib/baz.dart': '''
      import 'foo.dart';
      import 'quux.dart';
      class Baz implements Bar {
        const factory Baz.named() = Quux;
      }
      ''',
      'foo|lib/quux.dart': '''
      import 'baz.dart';
      class Quux implements Baz {
        const Quux();
      }
      ''',
    });
    expect(mocksContent,
        contains('void m([_i2.Bar? a = const _i3.Baz.named()]) =>'));
  });

The idea is:

  • the parameter comes from one library,
  • the factory constructor for the default value comes from a second library,
  • the constant value that the factory constructor gives comes from a third library.

I think the fix needs to be made in source_gen.

@kuhnroyal
Copy link
Contributor Author

Yea I didn't think of 3 levels/interface. The information is just not available at that point. I am not sure it would be correct for computeConstantValue() to return the type of the factory location. So there probably needs to be something new like Revivable.type which can be different from the DartObject which was created with computeConstantValue().

I still think the workaround via the ParameterElement may be helpful for a fair amount of these cases.

Copy link
Member

@srawlins srawlins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment

@srawlins
Copy link
Member

srawlins commented Aug 6, 2021

Would you mind rebasing once as well? Thanks!

* add tests
* gitignore build folder and generated mock test files
@kuhnroyal kuhnroyal force-pushed the feature/factory-type branch from 6675aff to d0dacca Compare August 7, 2021 10:43
@srawlins
Copy link
Member

srawlins commented Aug 7, 2021

I think your code became unformatted with the sync.

@kuhnroyal kuhnroyal force-pushed the feature/factory-type branch from d0dacca to 51b5d26 Compare August 8, 2021 11:27
@srawlins
Copy link
Member

Thanks much! I think this is definitely an improvement (especially with the real world example).

@srawlins srawlins merged commit 15ca5ad into dart-lang:master Aug 10, 2021
@kuhnroyal kuhnroyal deleted the feature/factory-type branch August 10, 2021 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Mocking a moor database fails

2 participants