Roll dwds to `24.0.0` and unified_analytics `6.1.0` #147250

eliasyishak · 2024-04-23T18:00:27Z

Rolling updates for dwds 24.0.0 and unified_analytics 6.1.0 and making the necessary fixes for the breaking changes

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.
I followed the breaking change policy and added Data Driven Fixes where supported.
All existing and new tests are passing.

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

christopherfujino · 2024-04-23T19:08:53Z

packages/flutter_tools/test/general.shard/resident_runner_test.dart

@@ -53,7 +53,7 @@ void main() {
  setUp(() {
    testbed = Testbed(setup: () {
      fakeAnalytics = getInitializedFakeAnalyticsInstance(
-        fs: globals.fs,
+        fs: MemoryFileSystem.test(),


Why did you make this change? It looks like this changes the semantics of this test.

With 6.1.0, the Analytics.fake constructor will only take a MemoryFileSystem, before it was a FileSystem which led to some bugs while i was migrating devtools because i didn't catch it was using my real filesystem.

And it turns out that globals.fs is not always a MemoryFileSystem for this test file which is why i swapped it out for a MemoryFileSystem... this works for the test because we don't touch the filesystem within the test

I see. I will have to think about this. This code seems non-ideal because we now have multiple file systems in a single test.

Is globals.fs a MemoryFileSystem? If so, maybe we should just do a cast.

I pushed a commit where the two testbed test libraries were re-written to just not use FakeAnalytics from package:unified_analytics: bc59013

More generally (and outside the scope of this PR), it might be advantageous in the tool if we just stopped importing FakeAnalytics from package:unified_analytics, if breaking changes to that class is going to necessitate these kinds of refactors.

Even more generally, we should try at all costs to avoid having duplicate instances of globals within a single test run (in this case FileSystem), as that leads to very difficult to reason about correctness bugs. This is especially easy to do with the TestBed, which is one of the reasons we stopped using it.

I pushed a commit where the two testbed test libraries were re-written to just not use FakeAnalytics from package:unified_analytics: bc59013

SGTM, this makes sense for those files since we aren't actually testing if an event was sent using the built in logic in package:unified_analytics

Even more generally, we should try at all costs to avoid having duplicate instances of globals within a single test run

I also agree with this, the change I had pushed most recently was using only one FileSystem, it was just a bit more work to get the delegate if we used the ThrowingForwardingFileSystem in that one test

More generally (and outside the scope of this PR), it might be advantageous in the tool if we just stopped importing FakeAnalytics from package:unified_analytics

I think there is value in using the exported FakeAnalytics because there may be tests that rely on the logic for package:unified_analytics to be correct. I do agree though that we don't really need to be using FakeAnalytics for tests that don't check for events sent, we could probably swap those with the NoOpAnalytics since we usually just add it to the class constructor being tested

there may be tests that rely on the logic for package:unified_analytics to be correct

Can you give concrete examples?

I do agree though that we don't really need to be using FakeAnalytics for tests that don't check for events sent

If we want to know which events were sent, we can use the fake implementation I have here, right? Or is there additional behavior we want to test for that we would not want to re-implement here?

Let me just file a separate issue where we can discuss independent of this roll :)

packages/flutter_tools/test/general.shard/isolated/resident_runner_test.dart

eliasyishak · 2024-04-23T21:25:13Z

packages/flutter_tools/test/general.shard/resident_runner_test.dart

+      // Logic to get the filesystem for analytics from globals
+      final ErrorHandlingFileSystem tempFs = globals.fs as ErrorHandlingFileSystem;
+      final MemoryFileSystem analyticsFileSystem;
+      if (tempFs.fileSystem is ThrowingForwardingFileSystem) {
+        analyticsFileSystem =
+            (tempFs.fileSystem as ThrowingForwardingFileSystem).fileSystem
+                as MemoryFileSystem;
+      } else {
+        analyticsFileSystem = tempFs.fileSystem as MemoryFileSystem;
+      }
      fakeAnalytics = getInitializedFakeAnalyticsInstance(
-        fs: globals.fs,
+        fs: analyticsFileSystem,
        fakeFlutterVersion: FakeFlutterVersion(),


@christopherfujino I'm not the biggest fan of this refactor but we had to consider two cases for this file

For most of the tests, we just rely on the ErrorHandlingFileSystem in which we pass the MemoryFileSystem.test constructor as the delegate, so getting the reference to the filesystem for the analytics constructor was fairly easy

There is one test in this file, HotRunner handles failure to write vmservice file, which uses ThrowingForwardingFileSystem for the delegate in ErrorHandlingFileSystem, as a result, I needed to conditionally check for both types before extracting the delegate or I would get the error below

type 'ThrowingForwardingFileSystem' is not a subtype of type 'MemoryFileSystem' in type cast

When there is no override in the test for FileSystem

ErrorHandlingFileSystem | --> delegate = MemoryFileSystem

When we override the FileSystem in the testUsingContext overrides we have an additional level of nesting

ErrorHandlingFileSystem | --> delegate = ThrowingFileSystem | --> delegate = MemoryFileSystem

Thanks for digging into this. Per my comment here, I think the best solution would be to just use an implementation of Analytics that does not rely on a file system at all. IMO, the file system operations of the Analytics class should be tested in package:unified_analytics--in tests for package:flutter_tools, all we should be testing is the events that we're sending it.

What about for those tests that rely on opt out status, for example, the new crash reporter refactor to use GA4 opt in status?

…lytics

eliasyishak · 2024-04-24T14:24:23Z

@DanTup do you have any idea why the integration tests are failing for the debug adapter here? Seems like something isn't getting printed out to stdout with these dependencies getting rolled

DanTup · 2024-04-24T14:34:45Z

@eliasyishak just debugging now... My first guess is that maybe dart-lang/sdk@717aa63 may have affected this - it made some output event async that weren't before (and it may have changed the order of output the tests are checking). If that is the case, I need to figure out if it's a bug or not (we must not re-order some kinds of events).

If you need to roll the other packages in the meantime, you could try pinning DAP to 4.1.0 which didn't have that change.

Edit: Indeed, the output looks like this:

<== [DAP] {"seq":42,"type":"event","body":{"category":"stdout","output":"topLevelFunction\n"},"event":"output"}
<== [DAP] {"seq":43,"type":"event","body":{"category":"stdout","output":""},"event":"output"}
<== [DAP] {"seq":45,"type":"event","body":{"category":"stdout","output":"topLevelFunction\n"},"event":"output"}
<== [DAP] {"seq":46,"type":"event","body":{"category":"stdout","output":""},"event":"output"}
<== [DAP] {"seq":50,"type":"event","body":{"category":"stdout","output":"Reloaded 0 libraries in 50ms (compile: 5 ms, reload: 0 ms, reassemble: 11 ms).\n"},"event":"output"}

Investigating why...

DanTup · 2024-04-24T14:47:12Z

Oh, I see the problem.. there's no ordering issue, but there are these empty output events:

<== [DAP] {"seq":43,"type":"event","body":{"category":"stdout","output":""},"event":"output"}

These don't really cause any real problems (because they're not doing anything) and are probably due to how we split up the output when scanning for stack traces. However since the test looks for output events that are not topLevelFunction, it's starting from the wrong place.

I'll send you a fix in the test to avoid this issue here, and I'll look into removing the (pointless) output events in a future DAP version.

See flutter#147250 (comment)

DanTup · 2024-04-24T14:58:19Z

@eliasyishak see eliasyishak#11

eliasyishak · 2024-04-24T15:15:01Z

Ah great catch! Thank you for debugging and getting something so quickly; going to review now!

Ignore empty output events in tests

Empty output events are harmless but unnecessary. The stack frame parsing code would always send one with the output ended with \n which until recently was only for stderr, but recently was enabled for stdout. The extra events broke some Flutter tests that were not expecting them. See flutter/flutter#147250 (comment) Change-Id: I13624f763d57a089d6b2d2c9e794cafb6a2f0023 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/364340 Commit-Queue: Ben Konyi <bkonyi@google.com> Commit-Queue: Helin Shiah <helinx@google.com> Reviewed-by: Ben Konyi <bkonyi@google.com> Reviewed-by: Helin Shiah <helinx@google.com>

eliasyishak · 2024-04-24T16:01:03Z

packages/flutter_tools/test/integration.shard/debug_adapter/test_client.dart

+        // Skip empty output events. These are pointless (but harmless) events
+        // that started occurring with pkg:dds 4.2.0 because of how the stack
+        // trace parsing is done. This will be unnecessary once fixed in
+        // DDS/DAP.
+        // TODO(dantup): Remove this once we're updated to a version of DAP that
+        //  includes https://dart-review.googlesource.com/c/sdk/+/364340.
+      .where((OutputEventBody output) => output.output.isNotEmpty)


Fix from @DanTup for breaking integration tests, error that was fixed below, thank you, Danny! :)

Expected: contains in order([<a string starting with 'Reloaded'>, 'topLevelFunction']) Actual: ['topLevelFunction'] Which: did not find a value matching a string starting with 'Reloaded' following expected prior values package:matcher expect test/integration.shard/debug_adapter/test_support.dart 44:5 expectLines test_support.dart:44 test/integration.shard/debug_adapter/flutter_adapter_test.dart 319:7 main.<fn>.<fn>

DanTup · 2024-04-24T16:16:31Z

Seems there are still some failures, taking a look...

 06:21 +43 ~3: loading test/integration.shard/debug_adapter/test_adapter_test.dart
 06:21 +43 ~3: test/integration.shard/debug_adapter/test_adapter_test.dart: (setUpAll)
 06:21 +43 ~3: test/integration.shard/debug_adapter/test_adapter_test.dart: integration tests can run in debug mode
 21:21 +43 ~3 -1: test/integration.shard/debug_adapter/test_adapter_test.dart: integration tests can run in debug mode [E]
   TimeoutException after 0:15:00.000000: Test timed out after 15 minutes.
   dart:isolate  _RawReceivePort._handleMessage

eliasyishak · 2024-04-24T16:24:11Z

@DanTup i think the timeout is getting stuck here: https://github.com/eliasyishak/flutter/blob/roll-dwds-and-analytics/packages/flutter_tools/test/integration.shard/debug_adapter/test_adapter_test.dart#L40-L46

DanTup · 2024-04-24T16:25:45Z

@eliasyishak yup, it seems like maybe the test process might not be ending/terminating correctly. I can repro locally, will debug.

DanTup · 2024-04-24T16:48:22Z

@eliasyishak based on the output, my feeling is that during the stack trace mapping we call lookupResolvedPackageUris but the VM terminates before it responds (because the tests completed) and we're not handling something correctly in that case.

If you're able to use the previous version of pkg:dds here short-term, that might let you get the analytics changes without this bug. I'll post back when I have an update though.

DanTup · 2024-04-24T17:57:23Z

Ok, I think I understand why this is failing now.. I'm not sure of the best fix and might need some input from Ben. I've opened dart-lang/sdk#55559 and will try to pick his brains.

elliette · 2024-04-29T22:53:56Z

Closing in favor of #147546

DDS was temporarily pinned to 4.1.0 because 4.2.0 triggered some test failures (see #147250). Those failures should be fixed by vm_service 14.2.2, so this unpins DDS and rolls both of these packages (along with devtools_shared, which is a DDS dependency). (If the bot updates vm_service before this is done, I can rebase over that will reduce the size of this PR to just a few files)

Update packages + fixes for breaking changes

65635f1

github-actions bot added tool Affects the "flutter" command-line tool. See also t: labels. a: desktop Running on desktop labels Apr 23, 2024

Fix analyze error + remove type cast

03bc61e

goderbauer mentioned this pull request Apr 23, 2024

Roll pub packages #147247

Closed

christopherfujino reviewed Apr 23, 2024

View reviewed changes

packages/flutter_tools/test/general.shard/isolated/resident_runner_test.dart Outdated Show resolved Hide resolved

eliasyishak added 4 commits April 23, 2024 16:24

Use local filesystem instead of global

138a2dc

Remove globals dependency from isolated/resident_runner_test.dart

a8f52d5

Swap back to using standalone instance for analytics

931601a

Logic added to extract the filesystem from globals

97e7438

eliasyishak commented Apr 23, 2024

View reviewed changes

eliasyishak added 2 commits April 23, 2024 17:30

Analyze error fix

c350891

Refactor to use type cast for isolated runner test

a5128e8

eliasyishak marked this pull request as ready for review April 24, 2024 12:12

Merge remote-tracking branch 'upstream/master' into roll-dwds-and-ana…

e588d98

…lytics

Ignore empty output events in tests

b58f013

See flutter#147250 (comment)

DanTup mentioned this pull request Apr 24, 2024

Ignore empty output events in tests eliasyishak/flutter#11

Merged

Update comment

515d695

Merge pull request #11 from DanTup/filter-empty-output-events

45f6e14

Ignore empty output events in tests

eliasyishak commented Apr 24, 2024

View reviewed changes

DanTup mentioned this pull request Apr 24, 2024

VmService does not fail in-flight requests when the connection closes unless dispose() is explicitly called dart-lang/sdk#55559

Closed

just use our own implementation of FakeAnalytics

bc59013

christopherfujino mentioned this pull request Apr 24, 2024

[flutter_tools] Stop instantiating FakeAnalytics from package:unified_analytics in tool tests #147330

Open

elliette mentioned this pull request Apr 29, 2024

Bump Flutter dependencies #147543

Closed

Pin DDS to 4.1.0 and re-run flutter update-packages --force-upgrade

28ebeed

andrewkolos assigned elliette Apr 29, 2024

elliette added a commit to elliette/flutter that referenced this pull request Apr 29, 2024

Copy of changes in flutter#147250

48b76fd

elliette mentioned this pull request Apr 29, 2024

Bump dependencies in Flutter #147546

Merged

elliette closed this Apr 29, 2024

DanTup mentioned this pull request May 7, 2024

Unpin DDS and roll pub packages #147925

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Roll dwds to `24.0.0` and unified_analytics `6.1.0` #147250

Roll dwds to `24.0.0` and unified_analytics `6.1.0` #147250

eliasyishak commented Apr 23, 2024 •

edited

christopherfujino Apr 23, 2024 •

edited

eliasyishak Apr 23, 2024

eliasyishak Apr 23, 2024 •

edited

christopherfujino Apr 23, 2024

christopherfujino Apr 23, 2024

christopherfujino Apr 24, 2024

christopherfujino Apr 24, 2024 •

edited

eliasyishak Apr 24, 2024 •

edited

christopherfujino Apr 24, 2024

christopherfujino Apr 24, 2024

eliasyishak Apr 23, 2024

eliasyishak Apr 23, 2024 •

edited

christopherfujino Apr 24, 2024

eliasyishak Apr 24, 2024

eliasyishak commented Apr 24, 2024

DanTup commented Apr 24, 2024 •

edited

DanTup commented Apr 24, 2024

DanTup commented Apr 24, 2024

eliasyishak commented Apr 24, 2024

eliasyishak Apr 24, 2024 •

edited

DanTup commented Apr 24, 2024

eliasyishak commented Apr 24, 2024

DanTup commented Apr 24, 2024

DanTup commented Apr 24, 2024

DanTup commented Apr 24, 2024

elliette commented Apr 29, 2024

Roll dwds to 24.0.0 and unified_analytics 6.1.0 #147250

Roll dwds to 24.0.0 and unified_analytics 6.1.0 #147250

Conversation

eliasyishak commented Apr 23, 2024 • edited

Pre-launch Checklist

christopherfujino Apr 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eliasyishak Apr 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christopherfujino Apr 24, 2024 • edited

Choose a reason for hiding this comment

eliasyishak Apr 24, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eliasyishak Apr 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eliasyishak commented Apr 24, 2024

DanTup commented Apr 24, 2024 • edited

DanTup commented Apr 24, 2024

DanTup commented Apr 24, 2024

eliasyishak commented Apr 24, 2024

eliasyishak Apr 24, 2024 • edited

Choose a reason for hiding this comment

DanTup commented Apr 24, 2024

eliasyishak commented Apr 24, 2024

DanTup commented Apr 24, 2024

DanTup commented Apr 24, 2024

DanTup commented Apr 24, 2024

elliette commented Apr 29, 2024

Roll dwds to `24.0.0` and unified_analytics `6.1.0` #147250

Roll dwds to `24.0.0` and unified_analytics `6.1.0` #147250

eliasyishak commented Apr 23, 2024 •

edited

christopherfujino Apr 23, 2024 •

edited

eliasyishak Apr 23, 2024 •

edited

christopherfujino Apr 24, 2024 •

edited

eliasyishak Apr 24, 2024 •

edited

eliasyishak Apr 23, 2024 •

edited

DanTup commented Apr 24, 2024 •

edited

eliasyishak Apr 24, 2024 •

edited