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.

Sign up for GitHub

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

Jump to bottom

Roll dwds to 24.0.0 and unified_analytics 6.1.0 #147250

Closed
wants to merge 14 commits into from
Closed

Roll dwds to 24.0.0 and unified_analytics 6.1.0 #147250

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
65635f1
Update packages + fixes for breaking changes
eliasyishak Apr 23, 2024
03bc61e
Fix analyze error + remove type cast
eliasyishak Apr 23, 2024
138a2dc
Use local filesystem instead of global
eliasyishak Apr 23, 2024
a8f52d5
Remove globals dependency from `isolated/resident_runner_test.dart`
eliasyishak Apr 23, 2024
931601a
Swap back to using standalone instance for analytics
eliasyishak Apr 23, 2024
97e7438
Logic added to extract the filesystem from globals
eliasyishak Apr 23, 2024
c350891
Analyze error fix
eliasyishak Apr 23, 2024
a5128e8
Refactor to use type cast for isolated runner test
eliasyishak Apr 23, 2024
e588d98
Merge remote-tracking branch 'upstream/master' into roll-dwds-and-ana…
eliasyishak Apr 24, 2024
b58f013
Ignore empty output events in tests
DanTup Apr 24, 2024
515d695
Update comment
DanTup Apr 24, 2024
45f6e14
Merge pull request #11 from DanTup/filter-empty-output-events
eliasyishak Apr 24, 2024
bc59013
just use our own implementation of FakeAnalytics
christopherfujino Apr 24, 2024
28ebeed
Pin DDS to 4.1.0 and re-run flutter update-packages --force-upgrade
elliette Apr 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/flutter_tools/lib/src/isolated/devfs_web.dart
View file
Open in desktop
Expand Up @@ -292,7 +292,7 @@ class WebAssetServer implements AssetReader {
},
toolConfiguration: ToolConfiguration(
loadStrategy: ddcModuleSystem
? FrontendServerLegacyStrategyProvider(
? FrontendServerDdcStrategyProvider(
ReloadConfiguration.none,
server,
PackageUriMapper(packageConfig),
Expand Down
16 changes: 8 additions & 8 deletions packages/flutter_tools/pubspec.yaml
View file
Open in desktop
Expand Up @@ -13,8 +13,8 @@ dependencies:
archive: 3.3.2
args: 2.5.0
browser_launcher: 1.1.1
dds: 3.4.0
dwds: 23.3.0
dds: 4.2.0
dwds: 24.0.0
completion: 1.0.1
coverage: 1.7.2
crypto: 3.0.3
Expand Down Expand Up @@ -52,7 +52,7 @@ dependencies:
http_multi_server: 3.2.1
convert: 3.1.1
async: 2.11.0
unified_analytics: 5.8.8+1
unified_analytics: 6.1.0

cli_config: 0.2.0
graphs: 2.3.1
Expand All @@ -77,9 +77,9 @@ dependencies:
clock: 1.1.1 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
csslib: 1.0.0 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
dap: 1.2.0 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
dds_service_extensions: 1.7.0 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
devtools_shared: 8.1.1 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
dtd: 2.1.0 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
dds_service_extensions: 2.0.0 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
devtools_shared: 9.0.1 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
dtd: 2.2.0 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
extension_discovery: 2.0.0 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
fixnum: 1.1.0 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
frontend_server_client: 4.0.0 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
Expand All @@ -100,7 +100,7 @@ dependencies:
sync_http: 0.3.1 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
term_glyph: 1.2.1 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
typed_data: 1.3.2 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
vm_service_interface: 1.0.1 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
vm_service_interface: 1.1.0 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
watcher: 1.1.0 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
web: 0.5.1 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
yaml_edit: 2.2.0 # THIS LINE IS AUTOGENERATED - TO UPDATE USE "flutter update-packages --force-upgrade"
Expand All @@ -120,4 +120,4 @@ dartdoc:
# Exclude this package from the hosted API docs.
nodoc: true

# PUBSPEC CHECKSUM: 1789
# PUBSPEC CHECKSUM: 3317
2 changes: 1 addition & 1 deletion packages/flutter_tools/test/commands.shard/hermetic/build_ios_test.dart
View file
Open in desktop
Expand Up @@ -70,7 +70,7 @@ final Platform notMacosPlatform = FakePlatform(
);

void main() {
late FileSystem fileSystem;
late MemoryFileSystem fileSystem;
late TestUsage usage;
late FakeAnalytics fakeAnalytics;
late BufferLogger logger;
Expand Down
2 changes: 1 addition & 1 deletion packages/flutter_tools/test/commands.shard/hermetic/build_ipa_test.dart
View file
Open in desktop
Expand Up @@ -72,7 +72,7 @@ class FakePlistUtils extends Fake implements PlistParser {
}

void main() {
late FileSystem fileSystem;
late MemoryFileSystem fileSystem;
late TestUsage usage;
late FakeProcessManager fakeProcessManager;
late ProcessUtils processUtils;
Expand Down
2 changes: 1 addition & 1 deletion packages/flutter_tools/test/commands.shard/hermetic/build_macos_test.dart
View file
Open in desktop
Expand Up @@ -64,7 +64,7 @@ final Platform notMacosPlatform = FakePlatform(
);

void main() {
late FileSystem fileSystem;
late MemoryFileSystem fileSystem;
late TestUsage usage;
late FakeProcessManager fakeProcessManager;
late ProcessUtils processUtils;
Expand Down
2 changes: 1 addition & 1 deletion packages/flutter_tools/test/commands.shard/hermetic/build_windows_test.dart
View file
Open in desktop
Expand Up @@ -45,7 +45,7 @@ final Platform notWindowsPlatform = FakePlatform(
);

void main() {
late FileSystem fileSystem;
late MemoryFileSystem fileSystem;
late ProcessManager processManager;
late TestUsage usage;
late FakeAnalytics fakeAnalytics;
Expand Down
2 changes: 1 addition & 1 deletion packages/flutter_tools/test/general.shard/android/android_gradle_builder_test.dart
View file
Open in desktop
Expand Up @@ -46,7 +46,7 @@ void main() {
late BufferLogger logger;
late TestUsage testUsage;
late FakeAnalytics fakeAnalytics;
late FileSystem fileSystem;
late MemoryFileSystem fileSystem;
late FakeProcessManager processManager;

setUp(() {
Expand Down
2 changes: 1 addition & 1 deletion packages/flutter_tools/test/general.shard/android/gradle_find_bundle_test.dart
View file
Open in desktop
Expand Up @@ -15,7 +15,7 @@ import '../../src/common.dart';
import '../../src/fakes.dart';

void main() {
late FileSystem fileSystem;
late MemoryFileSystem fileSystem;
late FakeAnalytics fakeAnalytics;

setUp(() {
Expand Down
2 changes: 1 addition & 1 deletion packages/flutter_tools/test/general.shard/build_system/targets/ios_test.dart
View file
Open in desktop
Expand Up @@ -42,7 +42,7 @@ const List<String> _kSharedConfig = <String>[

void main() {
late Environment environment;
late FileSystem fileSystem;
late MemoryFileSystem fileSystem;
late FakeProcessManager processManager;
late Artifacts artifacts;
late BufferLogger logger;
Expand Down
2 changes: 1 addition & 1 deletion packages/flutter_tools/test/general.shard/build_system/targets/macos_test.dart
View file
Open in desktop
Expand Up @@ -21,7 +21,7 @@ import '../../../src/fakes.dart';

void main() {
late Environment environment;
late FileSystem fileSystem;
late MemoryFileSystem fileSystem;
late Artifacts artifacts;
late FakeProcessManager processManager;
late File binary;
Expand Down
6 changes: 3 additions & 3 deletions packages/flutter_tools/test/general.shard/hot_test.dart
View file
Open in desktop
Expand Up @@ -109,7 +109,7 @@ void main() {

group('hotRestart', () {
final FakeResidentCompiler residentCompiler = FakeResidentCompiler();
late FileSystem fileSystem;
late MemoryFileSystem fileSystem;
late TestUsage testUsage;
late FakeAnalytics fakeAnalytics;

Expand Down Expand Up @@ -534,7 +534,7 @@ void main() {
});

group('hot attach', () {
late FileSystem fileSystem;
late MemoryFileSystem fileSystem;
late FakeAnalytics fakeAnalytics;

setUp(() {
Expand Down Expand Up @@ -578,7 +578,7 @@ void main() {
});

group('hot cleanupAtFinish()', () {
late FileSystem fileSystem;
late MemoryFileSystem fileSystem;
late FakeAnalytics fakeAnalytics;

setUp(() {
Expand Down
2 changes: 1 addition & 1 deletion packages/flutter_tools/test/general.shard/isolated/hot_test.dart
View file
Open in desktop
Expand Up @@ -27,7 +27,7 @@ import 'fake_native_assets_build_runner.dart';
void main() {
group('native assets', () {
late TestHotRunnerConfig testingConfig;
late FileSystem fileSystem;
late MemoryFileSystem fileSystem;
late FakeAnalytics fakeAnalytics;

setUp(() {
Expand Down
3 changes: 2 additions & 1 deletion packages/flutter_tools/test/general.shard/isolated/resident_runner_test.dart
View file
Open in desktop
Expand Up @@ -3,6 +3,7 @@
// found in the LICENSE file.


import 'package:file/memory.dart';
import 'package:flutter_tools/src/build_info.dart';
import 'package:flutter_tools/src/device.dart';
import 'package:flutter_tools/src/features.dart';
Expand Down Expand Up @@ -32,7 +33,7 @@ void main() {
setUp(() {
testbed = Testbed(setup: () {
fakeAnalytics = getInitializedFakeAnalyticsInstance(
fs: globals.fs,
fs: MemoryFileSystem.test(),
eliasyishak marked this conversation as resolved.
Show resolved Hide resolved
fakeFlutterVersion: FakeFlutterVersion(),
);

Expand Down
2 changes: 1 addition & 1 deletion packages/flutter_tools/test/general.shard/macos/cocoapods_test.dart
View file
Open in desktop
Expand Up @@ -29,7 +29,7 @@ enum _StdioStream {
}

void main() {
late FileSystem fileSystem;
late MemoryFileSystem fileSystem;
late FakeProcessManager fakeProcessManager;
late CocoaPods cocoaPodsUnderTest;
late BufferLogger logger;
Expand Down
6 changes: 6 additions & 0 deletions packages/flutter_tools/test/general.shard/resident_runner_helpers.dart
View file
Open in desktop
Expand Up @@ -172,6 +172,12 @@ class FakeDartDevelopmentServiceException implements dds.DartDevelopmentServiceE
@override
final String message;
static const String defaultMessage = 'A DDS instance is already connected at http://localhost:8181';

@override
Map<String, Object?> toJson() => <String, Object?>{
'error_code': errorCode,
'message': message,
};
}

class TestFlutterDevice extends FlutterDevice {
Expand Down
2 changes: 1 addition & 1 deletion packages/flutter_tools/test/general.shard/resident_runner_test.dart
View file
Open in desktop
Expand Up @@ -53,7 +53,7 @@ void main() {
setUp(() {
testbed = Testbed(setup: () {
fakeAnalytics = getInitializedFakeAnalyticsInstance(
fs: globals.fs,
fs: MemoryFileSystem.test(),
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

fakeFlutterVersion: FakeFlutterVersion(),
);

Expand Down
2 changes: 1 addition & 1 deletion packages/flutter_tools/test/general.shard/resident_web_runner_cold_test.dart
View file
Open in desktop
Expand Up @@ -29,7 +29,7 @@ import '../src/test_build_system.dart';
void main() {
late FakeFlutterDevice mockFlutterDevice;
late FakeWebDevFS mockWebDevFS;
late FileSystem fileSystem;
late MemoryFileSystem fileSystem;

setUp(() {
fileSystem = MemoryFileSystem.test();
Expand Down
2 changes: 1 addition & 1 deletion packages/flutter_tools/test/general.shard/resident_web_runner_test.dart
View file
Open in desktop
Expand Up @@ -102,7 +102,7 @@ void main() {
late FakeWebServerDevice webServerDevice;
late FakeDevice mockDevice;
late FakeVmServiceHost fakeVmServiceHost;
late FileSystem fileSystem;
late MemoryFileSystem fileSystem;
late ProcessManager processManager;
late TestUsage testUsage;
late FakeAnalytics fakeAnalytics;
Expand Down
3 changes: 1 addition & 2 deletions packages/flutter_tools/test/general.shard/unified_analytics_test.dart
View file
Open in desktop
Expand Up @@ -4,7 +4,6 @@

import 'package:file/memory.dart';
import 'package:flutter_tools/src/base/config.dart';
import 'package:flutter_tools/src/base/file_system.dart';
import 'package:flutter_tools/src/reporting/unified_analytics.dart';
import 'package:unified_analytics/unified_analytics.dart';

Expand All @@ -15,7 +14,7 @@ void main() {
const String userBranch = 'abc123';
const String clientIde = 'VSCode';

late FileSystem fs;
late MemoryFileSystem fs;
late Config config;
late FakeAnalytics analyticsOverride;

Expand Down
11 changes: 3 additions & 8 deletions packages/flutter_tools/test/src/common.dart
View file
Open in desktop
Expand Up @@ -17,7 +17,6 @@ import 'package:meta/meta.dart';
import 'package:path/path.dart' as path; // flutter_ignore: package_path_import
import 'package:test/test.dart' as test_package show test;
import 'package:test/test.dart' hide test;
import 'package:unified_analytics/testing.dart';
import 'package:unified_analytics/unified_analytics.dart';

import 'fakes.dart';
Expand Down Expand Up @@ -319,31 +318,27 @@ class FileExceptionHandler {
/// instance, then a second instance will be generated and returned. This second
/// instance will be cleared to send events.
FakeAnalytics getInitializedFakeAnalyticsInstance({
required FileSystem fs,
required MemoryFileSystem fs,
required FakeFlutterVersion fakeFlutterVersion,
String? clientIde,
String? enabledFeatures,
}) {
final Directory homeDirectory = fs.directory('/');
final FakeAnalytics initialAnalytics = FakeAnalytics(
final FakeAnalytics initialAnalytics = Analytics.fake(
tool: DashTool.flutterTool,
homeDirectory: homeDirectory,
dartVersion: fakeFlutterVersion.dartSdkVersion,
platform: DevicePlatform.linux,
fs: fs,
surveyHandler: SurveyHandler(homeDirectory: homeDirectory, fs: fs),
flutterChannel: fakeFlutterVersion.channel,
flutterVersion: fakeFlutterVersion.getVersionString(),
);
initialAnalytics.clientShowedMessage();

return FakeAnalytics(
return Analytics.fake(
tool: DashTool.flutterTool,
homeDirectory: homeDirectory,
dartVersion: fakeFlutterVersion.dartSdkVersion,
platform: DevicePlatform.linux,
fs: fs,
surveyHandler: SurveyHandler(homeDirectory: homeDirectory, fs: fs),
flutterChannel: fakeFlutterVersion.channel,
flutterVersion: fakeFlutterVersion.getVersionString(),
clientIde: clientIde,
Expand Down