Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

setUrlStrategy fails in integration test environment #116936

Open
kenzieschmoll opened this issue Dec 12, 2022 · 15 comments
Open

setUrlStrategy fails in integration test environment #116936

kenzieschmoll opened this issue Dec 12, 2022 · 15 comments
Labels
f: integration_test The flutter/packages/integration_test plugin framework flutter/packages/flutter repository. See also f: labels. P2 Important issues not at the top of the work list platform-web Web applications specifically team-web Owned by Web platform team triaged-web Triaged by Web platform team

Comments

@kenzieschmoll
Copy link
Member

This is called in the app's main method. In the integration test, this exception is thrown upon calling app.main():

══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞════════════════════════════════════════════════════
The following assertion was thrown running a test:
Assertion failed: org-dartlang-sdk:///lib/_engine/engine/window.dart:25:10
!_isUrlStrategySet
"Cannot set URL strategy more than once."

When the exception was thrown, this was the stack:
dart-sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/errors.dart 266:49      throw_
dart-sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/errors.dart 29:3        assertFailed
lib/_engine/engine/window.dart 25:11                                              set customUrlStrategy
lib/_engine/engine/initialization.dart 259:5                                      <fn>
dart-sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/operations.dart 367:37  _checkAndCall
dart-sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/operations.dart 372:39  dcall
packages/flutter_web_plugins/src/navigation/url_strategy.dart 41:19               setUrlStrategy
@kenzieschmoll kenzieschmoll added the f: integration_test The flutter/packages/integration_test plugin label Dec 12, 2022
@kenzieschmoll
Copy link
Member Author

This issue is a blocker for DevTools to be able to set up up integration tests. Explanation here. @yjbanov is there anyone on your team that can take a look at this to see why this value is already set in the engine for a web test environment?

@mdebbar
Copy link
Contributor

mdebbar commented Dec 16, 2022

I'm looking into this.

For background, we don't allow flutter web apps to set the url strategy more than once to prevent them from shooting themselves in the foot. Changing the url strategy is not something a normal app does. The expectation is that the app sets up the url strategy once on start up (or does nothing, in which case we default to the HashUrlStrategy).

Do integration tests run one after the other in the same instance? If that's the case then that explains the error you are seeing.

Solution:

What we can do is expose a unsetUrlStrategy function that you can call after each integration test to reset things.

@mdebbar
Copy link
Contributor

mdebbar commented Dec 16, 2022

Discussed this with the team, here's a brief summary:

The url strategy issue you're seeing is just a symptom of the fact that integration tests are NOT running in isolation. This leads to all kinds of weird behavior since the underlying state is preserved across tests. Ideally, you want each integration test to run in isolation (that's how real apps run, you don't run multiple apps consecutively in the same page).

// cc @yjbanov and @ditman for more thoughts.

@ditman
Copy link
Member

ditman commented Dec 16, 2022

How do these tests run?

Is there anything that can be reused from the engine test runner that can help here?

@mdebbar
Copy link
Contributor

mdebbar commented Dec 16, 2022

@ditman those tests work because on tearDown they call window.resetHistory() which resets the state of url strategy.

I'm not sure if they have access to this API from outside the engine. Maybe something like this would work?:

import 'dart:ui' as ui;

// On test tearDown:
(ui.window as dynamic).resetHistory();

@ditman
Copy link
Member

ditman commented Dec 16, 2022

Ahhh, cool trick! I think that should be doable, I hope that helps unblock this a little bit :S

@yjbanov
Copy link
Contributor

yjbanov commented Dec 16, 2022

The url strategy issue you're seeing is just a symptom of the fact that integration tests are NOT running in isolation. This leads to all kinds of weird behavior since the underlying state is preserved across tests. Ideally, you want each integration test to run in isolation (that's how real apps run, you don't run multiple apps consecutively in the same page).

The issue might be that testWidgets, used by package:integration_test, is designed to logically start its own new app. Having framework state transfer from on testWidgets to another is not a well-trodden path AFAIK, but I may be wrong. If we want to have one runApp and multiple tests interacting with it, then perhaps runApp should be called in a setUpAll and each testWidgets would exercise an already running app through a scenario. But I'm not sure if that's how package:integration_test was designed to work (cc @dnfield).

@darshankawar darshankawar added framework flutter/packages/flutter repository. See also f: labels. platform-web Web applications specifically labels Dec 29, 2022
@ditman
Copy link
Member

ditman commented Dec 29, 2022

@kenzieschmoll have you found a fix/workaround for this?

@ditman ditman added the waiting for customer response The Flutter team cannot make further progress on this issue until the original reporter responds label Dec 29, 2022
@kenzieschmoll
Copy link
Member Author

Adding this was an acceptable workaround 👍 Thanks!

  tearDown(() {
    (ui.window as dynamic).resetHistory();
  });

@github-actions github-actions bot removed the waiting for customer response The Flutter team cannot make further progress on this issue until the original reporter responds label Jan 10, 2023
@kenzieschmoll
Copy link
Member Author

Actually, it appears this workaround is flaky... it sometimes passes sometimes fails.

@mdebbar
Copy link
Contributor

mdebbar commented Jan 12, 2023

Ah sorry, this is my bad. I forgot that resetHistory is async.

Try this:

tearDown(() async {
  await (ui.window as dynamic).resetHistory();
});

@yjbanov yjbanov added the P2 Important issues not at the top of the work list label Jan 12, 2023
@kenzieschmoll
Copy link
Member Author

Ah adding the await seems to have fixed it. This is an acceptable workaround for now, thanks!

@kenzieschmoll
Copy link
Member Author

The proposed workaround was working for a while, but now I seem to be hitting an assertion error when calling resetHistory();

Assertion failed: org-dartlang-sdk:///lib/_engine/engine/navigation/history.dart:221:12
  _hasSerialCount(currentState) && _currentSerialCount == 0
  is not true
  dart-sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/errors.dart 288:49  throw_
  dart-sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/errors.dart 29:3    assertFailed
  lib/_engine/engine/navigation/history.dart 221:68                             tearDown
  dart-sdk/lib/_internal/js_dev_runtime/patch/async_patch.dart 45:50            <fn>
  dart-sdk/lib/async/zone.dart 1406:47                                          _rootRunUnary
  dart-sdk/lib/async/zone.dart 1307:19                                          runUnary
  dart-sdk/lib/async/future_impl.dart 147:18                                    handleValue
  dart-sdk/lib/async/future_impl.dart 767:44                                    handleValueCallback
  dart-sdk/lib/async/future_impl.dart 796:13                                    _propagateToListeners
  dart-sdk/lib/async/future_impl.dart 567:5                                     [_completeWithValue]
  dart-sdk/lib/async/future_impl.dart 640:7                                     <fn>
  dart-sdk/lib/async/zone.dart 1398:13                                          _rootRun
  dart-sdk/lib/async/zone.dart 1300:19                                          run
  dart-sdk/lib/async/zone.dart 1208:7                                           runGuarded
  dart-sdk/lib/async/zone.dart 1248:23                                          callback
  dart-sdk/lib/async/schedule_microtask.dart 40:11                              _microtaskLoop
  dart-sdk/lib/async/schedule_microtask.dart 49:5                               _startMicrotaskLoop
  dart-sdk/lib/_internal/js_dev_runtime/patch/async_patch.dart 177:15           <fn>

@mdebbar
Copy link
Contributor

mdebbar commented Jan 30, 2023

Taking a step back here. Do you really want your tests to mess with the real browser history and url? I'm assuming the answer is no. But if you don't do anything about it, and your tests perform any kind of flutter navigation, you'll be touching the real browser history.

You have 2 options here:

  1. Disable browser history completely. If you don't care about the browser history or url bar, then you can disable all of that by doing setUrlStrategy(null) (it's better to do this before runApp() or app.main()).

  2. If you do care about the history and the navigation state (e.g. if you are using flutter routes), then you probably should mock the url strategy so that you don't use the real browser history. Here's an example: https://github.com/flutter/flutter/blob/main/dev/integration_tests/web_e2e_tests/test_driver/url_strategy_integration.dart#L22-L22

@jaredsburrows
Copy link

jaredsburrows commented Jun 18, 2023

FWIW, you can wrap this line a a simple isTesting bool:

Main app code:

import 'package:flutter/material.dart';
import 'package:url_strategy/url_strategy.dart';

bool isTesting = false;

/// The main entry point of the application.
void main() {
  // Set the path URL strategy for Flutter web applications. This removes the
  // "#" symbol from the URL and makes it more user-friendly and easier to share.
  if (!isTesting) {
    setPathUrlStrategy();
  }

  runApp(const MyApp());
}

Test app code:

import 'package:flutter_test/flutter_test.dart';
import 'package:integration_test/integration_test.dart';
import 'package:<your-app>/main.dart' as app;

void main() {
  IntegrationTestWidgetsFlutterBinding.ensureInitialized();

  app.isTesting = true;

  testWidgets('test loading the app', (tester) async {
    // Build our app and trigger a frame.
    await app.main();
    await tester.pumpAndSettle();
  });
}

@flutter-triage-bot flutter-triage-bot bot added multiteam-retriage-candidate team-web Owned by Web platform team triaged-web Triaged by Web platform team labels Jul 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
f: integration_test The flutter/packages/integration_test plugin framework flutter/packages/flutter repository. See also f: labels. P2 Important issues not at the top of the work list platform-web Web applications specifically team-web Owned by Web platform team triaged-web Triaged by Web platform team
Projects
None yet
Development

No branches or pull requests

7 participants