Fix non-independant tests in binding_test.dart - #188963

I don't really understand what HttpOverrides.current has to do with this test. But it was there before, so I left it there. Let me know if you want me to remove it

Hmm it looks like it was added to verify that httpOverrides is initialized in TestWidgetsFlutterBinding. Could we pull expect(HttpOverrides.current, isNotNull) out of the testTextInput group and into its own test case to make sure we don't lose it? Because I agree, I don't think this is the right place for it.

I had to move things around a little bit.

HttpOverrides.current is being set by the first testWidgets if the file when it runs TestWidgetsFlutterBinding.ensureInitialized()

So I had to split the tests into 2 files:

• packages/flutter_test/test/http_overrides/test_test.dart
• packages/flutter_test/test/http_overrides/test_widgets_test.dart

Let me know what you think about it

Thanks! Looking at when the check was added (commit), I think we really just need to check that TestWidgetsFlutterBinding.ensureInitialized() registers the HttpOverrides. So I think instead of splitting the test cases out into separate files, we can probably add the following to bindings_test.dart and that should be sufficient to ensure we are not regressing on our test coverage:

  group('HttpOverrides', () {
    testWidgets('testWidgets registers HttpOverrides', (WidgetTester tester) async {
      expect(HttpOverrides.current, isNotNull);
    });
  });

Noted, I moved it back in Move back test in bindings_test.dart :)

The assertion expect(HttpOverrides.current, isNotNull); is unrelated to the testTextInput group. Including it in the setUp block of this group couples unrelated behaviors and means that any failure in HttpOverrides initialization will incorrectly cause all testTextInput tests to fail.

It is cleaner and more maintainable to separate this check into its own independent test case.

Hmm it looks like it was added to verify that httpOverrides is initialized in TestWidgetsFlutterBinding. Could we pull expect(HttpOverrides.current, isNotNull) out of the testTextInput group and into its own test case to make sure we don't lose it? Because I agree, I don't think this is the right place for it.

I realize this matches what we already had here, but could we change the test description to "testWidgets registers testTextInput" and "test does not register testTextInput" below? I think that helps make it clearer what is being tested.

Sure, I changed it in Move http overrides tests to its own file :)

Sorry for all the back and forth here, realizing the pre-existing test was actually checking that the AutomatedTestWidgetsFlutterBinding was registering httpOverrides. So maybe we should actually have 2 test cases for httpOverrides:

• this current one which will be outside of the AutomatedTestWidgetsFlutterBinding and uses testWidgets (matching (commit)
• another one inside the AutomatedTestWidgetsFlutterBinding group which uses test (matching test case in original "Initializes httpOverrides and testTextInput" case)

I'm not sure I understood this comment fully. Do you want me to re-apply this comment? Move http overrides tests to its own file

I cannot add a test like this

 test('testWidgets does not register HttpOverrides.current', () {
    expect(HttpOverrides.current, isNull);
  });

in the current file. As soon as a test binding is instantiated (by the current or another test), HttpOverrides.current will be non-null. That's because we are working with static methods here. And those are not cleaned up/unregistered at the end of the tests.

That's why I split them into different test files in Move http overrides tests to its own file

Having said that, does that change your comment? If not, could you rephrase it to help me understand it?

I meant we should add the following test case:

group(AutomatedTestWidgetsFlutterBinding, () {
 test('test registers HttpOverrides', () async {
   expect(HttpOverrides.current, isNotNull);
  });
    
   // more test cases...
})

Since there was a test case checking that before ("Initializes httpOverrides and testTextInput"). Does that make sense?

From what I understand,test does not register HttpOverrides.current. TestWidgetsFlutterBinding.ensureInitialized() does it.

testWidgets registers HttpOverrides.current because it calls TestWidgetsFlutterBinding.ensureInitialized() under the hood.

So I can either:

1. Re-apply this commit Move http overrides tests to its own file and verify test does NOT register HttpOverrides (it needs to be in its own file)
2. Add a test like so

  test('TestWidgetsFlutterBinding.ensureInitialized Initializes httpOverrides', () async {
    TestWidgetsFlutterBinding.ensureInitialized();
    expect(HttpOverrides.current, isNotNull);

and it would ideally be in its own file too.

Which option do you prefer?

Ah you're right, sorry about that! Re-applying Move http overrides tests to its own file sounds good!

No worries, I re-applied it in Move back http override tests to there own files

Definitely a red flag for non-independent tests.

nit: this should be "test does not register..."

Good catch, I fixed it in Move back http override tests to there own files

Ah you're right, sorry about that! Re-applying Move http overrides tests to its own file sounds good!