Fix error when resetting configurations in tear down phase

[fzyzcjy]

(This is WIP, since I want to see whether this change will make regression test fail or not) ready for review

Consider this simple example

import 'package:flutter/scheduler.dart';
import 'package:flutter_test/flutter_test.dart';

void main() {
  testWidgets('addTearDown should work', (tester) async { // <-- THIS FAILS
    timeDilation = 2;
    addTearDown(() => timeDilation = 1);
  });

  testWidgets('directly reset should work', (tester) async { // <-- this is ok
    timeDilation = 2;
    timeDilation = 1;
  });
}

It yields:

/Volumes/MyExternal/ExternalRefCode/flutter/bin/flutter --no-color test --machine --start-paused test/a.dart
Testing started at 09:50 ...

══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞════════════════════════════════════════════════════
The following assertion was thrown running a test:
The timeDilation was changed and not reset by the test.

When the exception was thrown, this was the stack:
#0      SchedulerBinding.debugAssertNoTimeDilation.<anonymous closure> (package:flutter/src/scheduler/binding.dart:662:9)
#1      SchedulerBinding.debugAssertNoTimeDilation (package:flutter/src/scheduler/binding.dart:665:6)
#2      TestWidgetsFlutterBinding._verifyInvariants (package:flutter_test/src/binding.dart:968:12)
#3      AutomatedTestWidgetsFlutterBinding._verifyInvariants (package:flutter_test/src/binding.dart:1433:11)
#4      TestWidgetsFlutterBinding._runTestBody (package:flutter_test/src/binding.dart:952:7)
<asynchronous suspension>

The test description was:
  addTearDown should work
════════════════════════════════════════════════════════════════════════════════════════════════════

Test failed. See exception logs above.
The test description was: addTearDown should work

In other words, we do not allow resetting configurations in addTearDown (or tearDown). Instead, we must do it at the end of the closure.

IMHO resetting things in addTearDown/tearDown is a commonly seen practice. For example, window.physicalSize can be reset in a tear-down function, and even Flutter test code inside the framework does so a lot of times. A quick search also shows that, such as this example, resetting in tear down holds even for other tests such as Python.

By disallowing so, Flutter devs may have a bit more friction when learning Flutter, since they may firstly write down code that follows common practice, realizing it does not work, and change it.

It is also inconsistent with other parts of the Flutter. As mentioned above, window.physicalSize can be reset in a tear-down function, but things like timeDilation cannot.

The PR can also make code a bit simpler. Originally, whenever writing setup code (e.g. timeDilation=2), we must put tear down code at the end of the function, and wrap with a try-finally. But with the PR, it can be put near the setup code, so it is a bit cleaner for code readers. By the way, this is also a bit like the "defer" keyword in go - something like configure(); defer reset(); other_functions() will let the reset be executed last.

In some cases, this seems to simplify code a lot. For example, in #113830 (comment), I have to introduce a weird timeDilation reset that seems to have no pairing timeDilation modification (and cause confusion of readers - even code reviewers). With this PR, the reset will be put to the next line of modification, so it is clear.

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
• I signed the CLA.
• I listed at least one issue that this PR fixes in the description above.
• I updated/added relevant documentation (doc comments with ///).
• I added new tests to check the change I am making, or this PR is test-exempt.
• All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

[goderbauer]

(This is WIP, since I want to see whether this change will make regression test fail or not)

Do you still have further plans for this? Or did it achieve its goal and can be closed?

[fzyzcjy]

Oh it is ready for review

[flutter-dashboard]

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

[pdblasi-google]

Looks like you found another interesting case here. I think you've got the right idea for fixing it, but I think we should take it further.

I did a bit of testing and we should be able to move the entire if statement starting at line 938 (if (_pendingExceptionDetails == null)) into an addTearDown that we register where you've got yours now.

That'll cover your use case, plus many other verifications that would run into a similar issue. And it wouldn't require splitting up the invariant verification methods as you've done here.

[fzyzcjy]

Thanks :) I will modify and update it soon (no time now though)

[pdblasi-google]

Followup on moving the invariant verifications into an addTearDown (came to me at about 3AM last night...).

I think if we move the invariant verifications into an addTearDown, we may also need to move the storing of those variables (for checking the end state) that get checked into a setUp call. I think as it stands right now, if someone were to use setUp to change an invariant, it'll have a couple problems:

a) It won't validate that the invariant is reset after the test, since it's actually set before the test
b) If (after this issue is fixed) the invariant is reset via an addTearDown or tearDown, then the verifications will fail because they'll have the changed value saved to check against, since it was changed before the test.

[fzyzcjy]

Good point! I will check that later.

[fzyzcjy]

Well, tearDown does not seem to work. I guess it is because, our addTearDown(verifyInvariants) are called before this user-added tearDown. Thus, even if user resets value in this tearDown, our verifications run before that so is not aware of it.

However, IMHO this PR is still useful, because even if tearDown is not supported, it adds support for addTearDown, which is very frequently used.

---

tests:

    group('tearDown does not work yet', () {
      tearDown(() {
        timeDilation = 1;
      });

      testWidgets('main test inside group', (WidgetTester tester) async {
        timeDilation = 2;
      });
    });

yields

package:flutter/src/scheduler/binding.dart 662:9  SchedulerBinding.debugAssertNoTimeDilation.<fn>
package:flutter/src/scheduler/binding.dart 665:6  SchedulerBinding.debugAssertNoTimeDilation
package:flutter_test/src/binding.dart 971:12      TestWidgetsFlutterBinding._verifyInvariants
package:flutter_test/src/binding.dart 1436:11     AutomatedTestWidgetsFlutterBinding._verifyInvariants
package:flutter_test/src/binding.dart 950:9       TestWidgetsFlutterBinding._runTestBody.<fn>
===== asynchronous gap ===========================
dart:async                                        _CustomZone.registerUnaryCallback
package:flutter_test/src/binding.dart 941:9       TestWidgetsFlutterBinding._runTestBody.<fn>

The timeDilation was changed and not reset by the test.

[fzyzcjy]

Hmm CI fails. I will firstly revert now

[pdblasi-google]

@fzyzcjy Apologies for the delay in followup.

It looks like the commit that failed CI only included the change to move most of the checks into the teardown, not moving the pre-test checks that grab the current values into a setup. As I mentioned in another comment, both changes would be needed to support the teardown for those additional cases since they compare against values that are (currently) set in the actual test.

Are you willing to keep pursuing those other validations being moved to the teardown as well, or should I file an issue for the rest and followup with that idea at a later date?

[fzyzcjy]

@pdblasi-google Hi thanks for the reply. I am quite busy recently, so it would be great to firstly merge a minimal PR and then do another PR later :)

[pdblasi-google]

@fzyzcjy Sounds good. I'll re-review here shortly with the intent to only support the current sub-set you have here and write up an issue for the rest of the variant checks.

[pdblasi-google]

LGTM

[pdblasi-google]

nit: This comment threw me off when I read it. I read this as "subclasses are intended to override this method" not as "existing subclasses in this library already override this method", which had me confused as to why we were intending users to override a private method. More explicit verbiage as to why we're keeping this method would be appreciated.

[goderbauer]

+1

[fzyzcjy]

Done!

[goderbauer]

LGTM after comment is updated.

[goderbauer]

+1

[fzyzcjy]

Hmm I already merge from latest master but CI is still unhappy...

[pdblasi-google]

I think around that time there we hit a rate limit for the service that does the .ci.yaml check. I remember seeing something about it in the discord. Should be good to go now, I'll add the autosubmit tag and you should be good!

[gnprice]

Is there an issue that tracks the problem this PR was aiming to solve? It'd be great to have it fixed, and I think an issue would be helpful on the way to that.

I see #121917 is related, but I think it's somewhat different: that would add a new function addTearDownWidgets(WidgetTesterCallback callback) for teardowns that need access to WidgetTester functionality, whereas this is about making the existing function addTearDown interact properly with the invariant checks.

If there isn't an issue, I'd be glad to file one — I'd basically copy-paste most of this PR description and make small edits (and of course credit where it came from.)

[pdblasi-google]

@gnprice

The "Modifications" section of that issue should enable setup and addTearDown to work appropriately. The root problem here is that testWidgets is doing a lot of not-testing in its test call, which leads to things not being set up early enough to use the test library's setup.

When just the actual test is run within the test call, this problem should go away.

[gnprice]

Ah excellent, good to know.

I think this PR has a nice clear problem statement for a particular problem which I'd like to be able to point to an issue to refer to, so I'll go ahead and file one. Then #121917 is a specific proposal that would solve this problem as well as accomplishing some other useful things, and I'll mention that relationship in the issue.