Sdk: Test coverage regression with const constructors

[rrousselGit]

On both stable and master channels of flutter, defines two files:

// main.dart
class Foo {
  const Foo();
}

// main_test.dart
import 'package:flutter_test/flutter_test.dart';

import 'package:example/main.dart';

void main() {
  test('Counter increments smoke test', () {
    const Foo();
  });
}

Then run flutter test --coverage

This will report the constructor of Foo to not be covered by tests when it is.

It seems like this issue happens if the constructor is used only with the const keyword. Changing the test to:

test('Counter increments smoke test', () {
  Foo();
});

correctly reports the constructor has covered.

[jensjoha]

/cc @askeksa-google my guess is that this is because of constant evaluation..? Is there any story for constants and coverage?

[mraleph]

This is working as expected. Constants are now evaluated in compile time so we don't have dynamic coverage information for them.

@a-siva I am not sure if we want to emulate old behaviour? Constant table is loaded lazily so we could actually traverse it while collecting coverage and infer from that which constants were accessed by the code that run. However constant table does not store enough information to give accurate coverage information (e.g. if class has multiple const constructors we have no way of knowing which constructor was used just by looking at the constant table - because it contains fully evaluated constants).

[rrousselGit]

This is working as expected. Constants are now evaluated in compile time so we don't have dynamic coverage information for them.

It used to work though. Why is that change _desired_?

The only way to fix this issue from a Dart developer point of view is to voluntarily _not_ use the const keyword in tests.

This is both unintuitive and will trigger a prefer_const lint.

And it's not that uncommon either.
On a single file example of provider, I have that issue happening 4 times.

[mraleph]

It used to work though. Why is that change desired?

Constants used to be lazily evaluated by the VM (and other back-ends), now it is Dart front-end that handles constant evaluation logic. This consolidates the implementations, simplifies back-ends and allows front-end to properly report compilation errors that were missing (or impossible to report) before.

This in turn allows faster language evolution. For example, without this change it would be rather complex to support constant UI-as-code expressions (const [a, ...b, c]), with proper errors messages and semantics - and it would require work in all backends. (That is why by the way UI-as-code was first released without support for constant expressions - because constant-update-2018 was not ready yet).

It would be honest to admit that we did not consider implications on the coverage information (and we did not have tests covering this particular case) - but even if we did, we would still do the change.

[a-siva]

This seems like a duplicate of https://github.com/flutter/flutter/issues/28542 which was fixed.

[mraleph]

@a-siva it is not a duplicate, though it is related. flutter/flutter#28542 was just about static const fields.

[rrousselGit]

Considering how --track-widget-creation works, would it be possible to do the same to solve this issue?

According to that comment, that flag works by adding invisible parameters on the constructor.

Could we add an invisible parameter that tracks which constructor was used?

[askeksa-google]

An invisible parameter could indeed tell you which constructor was used to produce a particular constant. However, this would break canonicalization of constants. In contrast to widgets (whose canonicalization you discussed in https://github.com/flutter/flutter/issues/44858) there are classes whose proper canonincalization are necessary for some subsystems to work.

Alternatively, a less precise form of coverage information could be provided, which only tells you whether a particular class is const instantiated somewhere in the program (and/or whether a constant expression containing such an instance was executed). I don't know to what extent such a feature is feasible in the Flutter coverage tool.

[rrousselGit]

However, this would break canonicalization of constants

Do you mind explaining how?

Because two identical consts will always use the same constructor.

This would transform:

const foo = Foo();
const foo2 = Foo();

const namedFoo = Foo.named();
const namedFoo2 = Foo.named();

into roughtly:

const foo = Foo(constructor: 'default');
const foo2 = Foo(constructor: 'default');

const namedFoo = Foo.named(constructor: 'named');
const namedFoo2 = Foo.named(constructor: 'named');

So from my understanding, both foo and foo2 should still be "identical".

Even in a situation where the const constructor uses ternary, like this:

class Foo {
  const Foo(int a) : a = a == 0 ? 0 : 42;

  final int a;
}

then adding a meta-data to track which branch of the ternary was reached should _still_ not break the canonicalization.

[askeksa-google]

Objects constructed using different constructors can end up being identical.

class Foo {
  const Foo() : a = 0;
  const Foo.named(this.a);

  final int a;
}

main() {
  const x = Foo();
  const y = Foo.named(0);
  print(identical(x, y));
}

prints true.

If metadata about the used constructor is added to the constants, they will no longer be identical.

[vanlooverenkoen]

Any update on this issue?

[mraleph]

We are not actively working to fix this. Closing this to reflect that we currently consider this working as intended.

[illia-romanenko]

Hi 👋,

unfortunately, we also facing the same issue described in this ticket. As @mraleph described there were reasons to make it that way but as it was mentioned:

we did not consider implications on the coverage information (and we did not have tests covering this particular case)

that really affects the smoothness of programming for all levels of experience. Coverage is an important metric - so it would be great if we could do something to make its collection reliable and that supports all of the Dart features.

If I may I want to bring attention to this issue for @timsneath @eseidelGoogle - hope we can have some solution.

Illia

[askeksa-google]

FWIW, here are my thoughts on what it would take (at minimum) to implement this:

• The Kernel format is extended such that all ConstantExpression nodes contain a list of the program locations of all expressions that were evaluated to produce that constant. This is a property of the constant expression, rather than the constant itself, since the same constant may arise through different computations.
• The constant evaluator keeps track of the locations of all expressions involved when it evaluates a constant and stores these into the resulting ConstantExpression. It will require some careful design to make sure that this does not cause unacceptable overhead in the situations where the feature is not used.
• When the VM reports coverage, it includes the locations associated with all executed constant expressions. This is so different from the way the VM currently tracks coverage that it essentially amounts to a separate coverage tracking mechanism on top of the existing one.

[vargavince91]

@mraleph Is there any chance of this ticket getting reopened?

I believe that the issue described by @rrousselGit is, in fact, still present and I consider the original issue a tooling problem. The explanation for the coverage report error is not trivial, and I would not consider this as "working as intended": the lines were called by the tests, so it should not be reported as "not covered".

I understand that the issue might not be the highest priority, or fixed in the next weeks, but I don't understand why it should be closed.

Just imagine you write your tests, see that some part of the code is not covered, even though your intuition would suggest otherwise. Then you spend time tracking down the issue, then introduce workarounds (not calling it with const), which in turn might require that you add exceptions to your listing rules in your test directory. Then, the same happens to your coworker: see and solve issue, continue workarounds... Then a third coworker deletes those because he/she doesn't understand why the lint exceptions were added and a const constructor is called without a const...

What I'm trying to say that it's an annoying issue and, based on @askeksa-google's comment, there might be a solution.

[mraleph]

the lines were called by the tests, so it should not be reported as "not covered".

The lines were not really "called" by tests in the same sense the rest of your code was called. For example:

class X {
  const X();
}

void main() {
  test('a', () {
    const X();
  });

  test('b', () {
  });
}

const X() would be evaluated by the front-end whenever you parse this file - it does not matter whether you will end up running test a or b or neither or them.

In the old world const X() would actually be executed in the backend and only if you actually run test a.

So if you look at the coverage as "report which parts of the program actually executed" then not reporting the constructor as executed is absolutely correct.

What I'm trying to say that it's an annoying issue and, based on @askeksa-google's comment, there might be a solution.

Yes, there is a solution, however it is rather complex and cross cutting. We might address this at some point in the future, but currently it does not look like this is something we are going to prioritise working on.

[rrousselGit]

So if you look at the coverage as "report which parts of the program actually executed" then not reporting the constructor as executed is absolutely correct.

But in that sense, since the file compiled, it was executed.

It was executed during compilation but still executed. Otherwise, the following code would run:

class Example {
  const Example(): assert(false);
} 

void main( ){
  const Example();
}

but we currently get a compilation error.

[jensjoha]

Here's an idea:
What if the constant evaluator kept track of all the places it evaluated during the constant evaluation and communicated that to the VM as "pre-executed"?

This _is_ different from before, though, as for instance code like this:

main() {
  foo(false);
}

foo(bool b) {
  if (b) bar();
}

bar() {
  print(const Baz(42));
}

class Baz {
  final x;
  const Baz(this.x);
  String toString() => "Baz[$x]";
}

would mark the Baz constructor as having run because the compiler evaluated it - even though it wouldn't have been marked as run before because the code-path was not executed.

It is technically true, though, as it was already evaluated and thus logically executed --- just by the compiler rather than the VM.

Would this satisfy your needs?

[rrousselGit]

That sounds good to me