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

Plumbing flags required for running tests with the DDC module system. #2295

Merged
merged 29 commits into from Jan 11, 2024

Conversation

Markzipan
Copy link
Contributor

@Markzipan Markzipan commented Dec 1, 2023

Besides passing flags, this change includes:

  • Forking dwds/test/frontend_server_evaluate_sound_test.dart as dwds/test/frontend_server_ddc_evaluate_sound_test. This helps us avoid timeouts.
  • Adding additional synchronization layers in DWDS's TestContext. This fixes a race condition I encountered (replicable by adding a Future.delayed(someDuration) just before getTab).
  • Exporting functions in ddc_names.dart as dwds/lib/utilities.dart
  • Adding some synchronization logic that enforces the order that we establish a tab connection, establish an app connection, and run main. Fixes [dwds] Race condition when establishing a tab connection and processing socket connections/requests in eval tests. #2298

This change requires the updated DDC Module Loader API to be landed first in the SDK (see this change).

This is part of a greater effort to deprecate the AMD module system: dart-lang/sdk#52361

Copy link
Contributor

@annagrin annagrin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Mark, looks like a great step in the right direction for module system unification! I left some comments.

dwds/lib/src/services/expression_compiler.dart Outdated Show resolved Hide resolved
dwds/test/fixtures/context.dart Outdated Show resolved Hide resolved
dwds/test/fixtures/context.dart Outdated Show resolved Hide resolved
frontend_server_common/lib/src/asset_server.dart Outdated Show resolved Hide resolved
frontend_server_common/lib/src/devfs.dart Outdated Show resolved Hide resolved
test_common/lib/test_sdk_layout.dart Outdated Show resolved Hide resolved
test_common/lib/test_sdk_layout.dart Outdated Show resolved Hide resolved
test_common/lib/test_sdk_layout.dart Outdated Show resolved Hide resolved
test_common/lib/test_sdk_layout.dart Outdated Show resolved Hide resolved
test_common/lib/test_sdk_layout.dart Outdated Show resolved Hide resolved
Copy link
Contributor

@annagrin annagrin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Mark, left a few more comments

dwds/lib/src/services/expression_compiler.dart Outdated Show resolved Hide resolved
dwds/lib/src/services/expression_compiler.dart Outdated Show resolved Hide resolved
dwds/test/fixtures/context.dart Outdated Show resolved Hide resolved
dwds/test/fixtures/context.dart Outdated Show resolved Hide resolved
dwds/test/fixtures/context.dart Outdated Show resolved Hide resolved
test_common/lib/test_sdk_layout.dart Outdated Show resolved Hide resolved
dwds/test/fixtures/utilities.dart Outdated Show resolved Hide resolved
test_common/lib/sdk_asset_generator.dart Outdated Show resolved Hide resolved
test_common/lib/sdk_asset_generator.dart Outdated Show resolved Hide resolved
test_common/lib/test_sdk_configuration.dart Outdated Show resolved Hide resolved
Copy link
Contributor

@annagrin annagrin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really good. Let's separate the major API update into a separate PR and add a few tests. Thanks!

target: project.directoryToServe,
buildResults: buildResults,
chromeConnection: () async => connection,
autoRun: testSettings.autoRun,
completeBeforeHandlingConnections: tabConnectionCompleter.future,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am curious about the exact details here (given that the plumbing changes the interface and will need a major version update) - could you please add more information on what exactly is the set of events and how it hangs? Also adding the comments in code so we can avoid this in the future.

dwds/lib/dart_web_debug_service.dart Outdated Show resolved Hide resolved
dwds/test/fixtures/context.dart Show resolved Hide resolved
frontend_server_common/lib/src/bootstrap.dart Show resolved Hide resolved
frontend_server_common/lib/src/bootstrap.dart Show resolved Hide resolved
frontend_server_common/lib/src/frontend_server_client.dart Outdated Show resolved Hide resolved
test_common/lib/sdk_asset_generator.dart Outdated Show resolved Hide resolved
Copy link
Contributor

@annagrin annagrin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank @Markzipan, this is looking good! lets fix some minor things and add a couple of tests (I left comments).

dwds/lib/src/services/expression_compiler.dart Outdated Show resolved Hide resolved
frontend_server_common/lib/src/bootstrap.dart Show resolved Hide resolved
Copy link
Contributor

@annagrin annagrin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Markzipan, just a few things I missed plus an ask for one more test.

dwds/test/fixtures/context.dart Outdated Show resolved Hide resolved
dwds/test/fixtures/context.dart Show resolved Hide resolved
@annagrin
Copy link
Contributor

annagrin commented Jan 9, 2024

Looks good! Let's sync and make sure tests are passing.

@annagrin annagrin self-requested a review January 9, 2024 00:24
Copy link
Contributor

@annagrin annagrin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Mark, LGTM!

@Markzipan Markzipan merged commit 10131b1 into dart-lang:master Jan 11, 2024
47 checks passed
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Feb 24, 2024
…#141423)

### Context:

DDC modules are abstractions over how libraries are loaded/updated. The entirety of google3 uses the DDC/legacy module system due to its flexibility extensibility over the other two (ES6 and AMD/RequireJS). Unifying DDC's module system saves us from duplicating work and will allow us to have finer grained control over how JS modules are loaded. This is a a prerequisite to features such as hot reload.

### Overview:

This change plumbs a boolean flag through flutter_tools that switches between DDC (new) and AMD (current) modules. This mode is automatically applied when `--extra-front-end-options=--dartdevc-module-format=ddc` is specified alongside `flutter run`. Other important additions include:
* Splitting Flutter artifacts between DDC and AMD modules
* Adding unit tests for the DDC module system
* Additional bootstrapper logic for the DDC module system

We don't expect to see any user-visible behavior or performance differences.

This is dependent on [incoming module system support in DWDS](dart-lang/webdev#2295) and [additional artifacts in the engine](flutter/engine#47783).

This is part of a greater effort to deprecate the AMD module system: dart-lang/sdk#52361
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants