dart-lang / mockito

Mockito-inspired mock library for Dart
https://pub.dev/packages/mockito
Apache License 2.0
632 stars 162 forks source link

`bool.fromEnvironment('dart.library.js_util')` (and alike?) constant can't be revived, breaking the codegen #683

Open dustincatap opened 1 year ago

dustincatap commented 1 year ago

Hi. I don't know if this issue is valid for this repository.

I am having trouble generating mocks when using auto_route.

Running dart run build_runner build --verbose outputs this error:

[INFO] Entrypoint:Generating build script...
[INFO] Entrypoint:Generating build script completed, took 287ms

[INFO] BuildDefinition:Initializing inputs
[INFO] BuildDefinition:Reading cached asset graph...
[INFO] BuildDefinition:Reading cached asset graph completed, took 46ms

[INFO] BuildDefinition:Checking for updates since last build...
[INFO] BuildDefinition:Checking for updates since last build completed, took 464ms

[INFO] Build:Running build...
[INFO] Build:Running build completed, took 9ms

[INFO] Build:Caching finalized dependency graph...
[INFO] Build:Caching finalized dependency graph completed, took 25ms

[SEVERE] mockito:mockBuilder on test/main_test.dart (cached):

Bad state: No element
dart:core                                               List.first
package:source_gen/src/constants/revive.dart 104:21     reviveInstance
package:source_gen/src/constants/reader.dart 278:25     _DartObjectConstant.revive
package:mockito/src/builder.dart 393:34                 _TypeVisitor._addTypesFromConstant
package:mockito/src/builder.dart 289:7                  _TypeVisitor.visitParameterElement
package:analyzer/src/dart/element/element.dart 5560:15  ParameterElementImpl.accept
package:analyzer/src/dart/element/element.dart 2881:13  ElementImpl.visitChildren
package:analyzer/dart/element/visitor.dart 323:13       RecursiveElementVisitor.visitMethodElement
package:mockito/src/builder.dart 276:11                 _TypeVisitor.visitMethodElement
package:analyzer/src/dart/element/element.dart 4896:54  MethodElementImpl.accept
package:analyzer/src/dart/element/element.dart 2881:13  ElementImpl.visitChildren
package:analyzer/dart/element/visitor.dart 233:13       RecursiveElementVisitor.visitClassElement
package:mockito/src/builder.dart 258:11                 _TypeVisitor.visitClassElement
package:analyzer/src/dart/element/element.dart 826:20   ClassElementImpl.accept
package:mockito/src/builder.dart 160:20                 MockBuilder._resolveAssetUris.addTypesFrom
package:mockito/src/builder.dart 168:9                  MockBuilder._resolveAssetUris.addTypesFrom
package:mockito/src/builder.dart 173:7                  MockBuilder._resolveAssetUris
package:mockito/src/builder.dart 76:29                  MockBuilder.build

[SEVERE] Build:
Failed after 63ms

Here is a sample project

yanok commented 1 year ago

I can reproduce with just

class X {
  void m({bool b = kIsWeb}) { print(b); }
}

(package:auto_route uses !kIsWeb as a default value, but just kIsWeb is enough).

The thing is kIsWeb is defined using bool.fromEnvironment and I guess Analyzer has no chance of knowing what the environment is.

@srawlins Could you please confirm this never worked and this is not a regression?

yanok commented 1 year ago

Corr: it's not just bool.fromEnvironment but specifically bool.fromEnvironment('dart.library.js_util') that results in bool (-unknown-) value.

As a quick fix we could probably detect this and try just referencing the constant instead. Won't work with !kIsWeb though.

Longer term I had an idea how we could get rid of the need to revive default values:

That's just a sketch but I think it could be made to work. At the same time it seems a bit too complex.

So probably even a longer term we should do a breaking API change and separate mocks from their configuration objects as Lasse proposed long ago. So config object will be generated but it won't need any default args. And mocks won't have any overrides except for nSM, so they will get the right default values for free.

yanok commented 1 year ago

@dustincatap For you one possible fix will be to wrap RootStackRouter in a class of your own (making sure you don't have kIsWeb as an argument default value) and mock that instead. So far I think that's the only option.

We could come up with some quick fix, but I can't promise that. And a longer term solution won't be implemented any time soon.