Parser machine logs #118707

Jasguerrero · 2023-01-18T16:30:25Z

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].
All existing and new tests are passing.

christopherfujino · 2023-01-24T18:11:15Z

dev/bots/test.dart

+  try {
+    final File info = fileSystem.file(path.join(flutterRoot, 'error.log'));
+    info.writeAsStringSync(json.encode(test.errors));
+  } on fs.FileSystemException catch (e){


what kind of file system exceptions do you anticipate? and why should we silently (to my knowledge, nobody actually checks the logs on green builds) swallow them?

This is me planing for the worst and just caching a file system exception because in theory it can happen https://api.flutter.dev/flutter/dart-io/File/writeAsStringSync.html; The reason I don't fail this is because if it didn't fail at dart run test --file-reporter... it means that the failure only occurs after trying to write to a file, something unrelated to the PR in testing

And in the case dart run test failed first and later on this failed too the we will have a red build

If we fail writing this file, will the recipe fail later on trying to read it?

Good question, I think it should fail

then better to fail fast with a very specific error message

So you suggest raising an exception in here stating that something failed in creating the error.log file?

Yes. I don't know if there is value in catching this exception if the recipe will just fail later. Then someone investigating a broken tree might think there is something wrong with the recipe, when really the problem was earlier and we could have left clues as the real location of the issue.

Fair enough

christopherfujino · 2023-01-24T22:19:24Z

dev/bots/tool_subsharding.dart

+}
+
+/// Intended to parse the output file of `dart test --file-reporter json:file_name
+TestFileReporterResults parseFileReporter(File metrics) {


style nit: this function's whole purpose is to take input values, process them, and then return an initialized instance of a class--the paradigm with object-oriented programming is to do this from a constructor. Since you need to keep state as you're parsing, it makes sense to track this state in a factory, and then at the end, once you have all your state initialized, call a hidden private constructor from that factory, like:

class TestFileReporterResults { TestFileReporterResults._({ required this.allTestSpecs, required this.hasFailedTests, required this.errors, }); /// Intended to parse the output file of `dart test --file-reporter json:file_name factory TestFileReporterResults.fromFile(File metrics) { if (!metrics.existsSync()) { throw Exception('${metrics.path} does not exist'); } final Map<int, TestSpecs> testSpecs = <int, TestSpecs>{}; bool hasFailedTests = true; final List<String> errors = <String>[]; for(final String metric in metrics.readAsLinesSync()) { final Map<String, dynamic> entry = json.decode(metric) as Map<String, dynamic>; if (entry.containsKey('suite')) { final Map<dynamic, dynamic> suite = entry['suite'] as Map<dynamic, dynamic>; addTestSpec(suite, entry['time'] as int, testSpecs); } else if (isMetricDone(entry, testSpecs)) { final Map<dynamic, dynamic> group = entry['group'] as Map<dynamic, dynamic>; final int suiteID = group['suiteID'] as int; addMetricDone(suiteID, entry['time'] as int, testSpecs); } else if (entry.containsKey('error') && entry.containsKey('stackTrace')) { errors.add('${entry['error']}\n ${entry['stackTrace']}'); } else if (entry.containsKey('success') && entry['success'] == true) { hasFailedTests = false; } } return TestFileReporterResults._(allTestSpecs: testSpecs, hasFailedTests: hasFailedTests, errors: errors); } final Map<int, TestSpecs> allTestSpecs; bool hasFailedTests; final List<String> errors; static void addTestSpec(Map<dynamic, dynamic> suite, int time, Map<int, TestSpecs> allTestSpecs) { allTestSpecs[suite['id'] as int] = TestSpecs( path: suite['path'] as String, startTime: time, ); } static void addMetricDone(int suiteID, int time, Map<int, TestSpecs> allTestSpecs) { final TestSpecs testSpec = allTestSpecs[suiteID]!; testSpec.endTime = time; } static bool isMetricDone(Map<String, dynamic> entry, Map<int, TestSpecs> allTestSpecs) { if (entry.containsKey('group') && entry['type'] as String == 'group') { final Map<dynamic, dynamic> group = entry['group'] as Map<dynamic, dynamic>; return allTestSpecs.containsKey(group['suiteID'] as int); } return false; } }

In addition, I can see you're typing the suite as Map<dynamic, dynamic>--can the key really be anything other than a string? Also, prefer Object? over dynamic, per https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#avoid-using-var-and-dynamic

Sounds good; I wasn't able to find info on what types can be returned from --file-reporter thats why I decided to go with Map<dynamic, dynamic> rather than anything else, I can change this to Object? as per style

If it's JSON, the spec only allows strings as the keys: https://www.json.org/json-en.html

christopherfujino · 2023-01-25T22:02:05Z

dev/bots/test.dart

+    final File info = fileSystem.file(path.join(flutterRoot, 'error.log'));
+    info.writeAsStringSync(json.encode(test.errors));
+  } on fs.FileSystemException catch (e){
+    throw fs.FileSystemException('Failed to generate info file (error.log): $e');


I think it's enough to just not catch this (I'm pretty sure the exception message will have details on exactly what it was trying to do when it failed)

And by not catching it, you'll get a more accurate stacktrace

christopherfujino · 2023-01-26T17:46:33Z

dev/bots/tool_subsharding.dart

+    bool hasFailedTests = true;
+    final List<String> errors = <String>[];
+
+    for(final String metric in metrics.readAsLinesSync()) {


Suggested change

for(final String metric in metrics.readAsLinesSync()) {

for (final String metric in metrics.readAsLinesSync()) {

christopherfujino · 2023-01-26T17:47:39Z

dev/bots/tool_subsharding.dart

+        final Map<String, Object?> group = entry['group']! as Map<String, Object?>;
+        final int suiteID = group['suiteID']! as int;
+        addMetricDone(suiteID, entry['time']! as int, testSpecs);
+      } else if (entry.containsKey('error') && entry.containsKey('stackTrace')) {


shouldn't we still track if it had an error but no stacktrace?

christopherfujino

two nits, otherwise LGTM

* 254a796 Revert "Reland "Add --serve-observatory flag to run, attach, and test (#118402)" (#119529)" (flutter/flutter#119729) * 1573c12 Marks Mac_android microbenchmarks to be unflaky (flutter/flutter#119727) * 5613ab0 remove unnecessary parens (flutter/flutter#119736) * 1305a50 Roll Flutter Engine from c08a286d60e9 to ba3adb74d952 (5 revisions) (flutter/flutter#119741) * 484d881 [conductor] update console link (flutter/flutter#118338) * 73124dc Fix `ListTileThemeData.copyWith` doesn't override correct properties (flutter/flutter#119738) * bc8927c 2b8dd4e5c Use Windows high contrast black/white theme with `MaterialApp` themes (flutter/engine#39206) (flutter/flutter#119747) * 578edfc Catch errors thrown while handling pointer events (flutter/flutter#119577) * 8fd5d4e Remove deprecated SystemNavigator.routeUpdated method (flutter/flutter#119187) * 7a926dc Deprecate MediaQuery[Data].fromWindow (flutter/flutter#119647) * cd118da Update a test expectation that depended on an SkParagraph fix (flutter/flutter#119756) * 4ae2d3b 🔥 Do not format the messages file for `gen-l10n` (flutter/flutter#119596) * 8f5949e Roll Flutter Engine from 2b8dd4e5c699 to a12d102773dd (2 revisions) (flutter/flutter#119758) * 4c99da6 Avoid printing blank lines between "Another exception was thrown:" messages. (flutter/flutter#119587) * 2ecf4ae Update the counter app to enable Material 3 (flutter/flutter#118835) * 0b8486d 29d09845f Roll Dart SDK from b47964e5d575 to 1c219a91e637 (1 revision) (flutter/engine#39324) (flutter/flutter#119763) * ca7b7e3 Roll Flutter Engine from 29d09845f21e to 97d27ff59459 (2 revisions) (flutter/flutter#119765) * 475fc4a Run Mac hostonly tests on any available arch (flutter/flutter#119762) * 322d10e 18118e10a Add iOS spring animation objc files (flutter/engine#38801) (flutter/flutter#119768) * f767f86 check if directory exists before listing content (flutter/flutter#119748) * 34730c7 Revert "Run Mac hostonly tests on any available arch (#119762)" (flutter/flutter#119784) * f9daa9a Roll Flutter Engine from 18118e10a0a5 to 679c4b42e222 (4 revisions) (flutter/flutter#119783) * fd76ef0 Reland "Add API for discovering assets" (flutter/flutter#119277) * ca0596e Fix `pub get --unknown-flag` (flutter/flutter#119622) * 9abb6d7 Roll Plugins from 9da327c to 9302d87 (11 revisions) (flutter/flutter#119825) * 9eafbcc Roll Flutter Engine from 679c4b42e222 to 388890a98e5b (6 revisions) (flutter/flutter#119830) * 1ee8799 Revert "[Re-land] Exposed tooltip longPress (#118796)" (flutter/flutter#119832) * 07b51a0 Add missing variants and *new* indicators to `useMaterial3` docs (flutter/flutter#119799) * 201380a e9e601c7c [web] Hide autofill overlay (flutter/engine#39294) (flutter/flutter#119833) * 1c0065c 27f55219d Roll Fuchsia Linux SDK from QxkjqmRgowkk_n2NZ... to pWloCaRzjLEAUvQEz... (flutter/engine#39339) (flutter/flutter#119838) * e7d934a Marks Linux_android android_choreographer_do_frame_test to be flaky (flutter/flutter#119721) * 3f986e4 Roll Flutter Engine from 27f55219df79 to ae38c9585a61 (2 revisions) (flutter/flutter#119840) * b0f1714 Make Flex,Row,Column const for real (flutter/flutter#119673) * d4b6898 [web] Put all index.html operations in one place (flutter/flutter#118188) * d63987f Parser machine logs (flutter/flutter#118707) * 22bbdf0 Android defines target update (flutter/flutter#119766) * 8387c23 [flutter_tools] Use base DAP detach and ensure correct output (flutter/flutter#119076) * d820aec Manual pub roll with dwds fix (flutter/flutter#119575) * 72f9cf5 Roll Flutter Engine from ae38c9585a61 to 616ecd8be3de (3 revisions) (flutter/flutter#119859) * cfdc358 roll packages (flutter/flutter#119865) * d875899 Bump test Chrome version for Mac arm support (flutter/flutter#119773) * 9b86a48 Fix gets removedItem instead of its index (flutter/flutter#119638) * 66b2ca6 Roll Flutter Engine from 616ecd8be3de to 2871970337df (3 revisions) (flutter/flutter#119870) * c626460 Make `_focusDebug` not interpolate in debug mode (flutter/flutter#119680) * a27802e flutter_tool: remove explicit length header in HTTP response (flutter/flutter#119869) * 3570cce Remove deprecated kind in GestureRecognizer et al (flutter/flutter#119572) * bc45b18 2696fff87 Roll Skia from c2d81db3ef41 to 4f0166baf5a4 (13 revisions) (flutter/engine#39348) (flutter/flutter#119879) * f3effce Roll Flutter Engine from 2696fff8716d to e3fe6dade964 (3 revisions) (flutter/flutter#119892) * 69421c1 [framework] use shader tiling instead of repeated calls to drawImage (flutter/flutter#119495) * 6b83eff Roll Flutter Engine from e3fe6dade964 to 655530e3fd15 (5 revisions) (flutter/flutter#119905) * a5d8a4a 67d35267c [Impeller] Use minimal coverage for stencil restores after overdraw prevention (flutter/engine#39358) (flutter/flutter#119910) * fc8ea56 0fb48ce5b Roll Dart SDK from 69452c5012d9 to be795cc64bd7 (1 revision) (flutter/engine#39360) (flutter/flutter#119926) * e0b2138 Dispose OverlayEntry in TooltipState. (flutter/flutter#117291) * c5e8757 Add M3 support for iconbuttons in error state in TextFields (flutter/flutter#119925)

Jasguerrero added 2 commits January 18, 2023 10:07

remove file reporter optional

cd3c954

decouple stack trace from metric collections

b7bd4d9

flutter-dashboard bot added the c: contributor-productivity Team-specific productivity, code health, technical debt. label Jan 18, 2023

Jasguerrero added 5 commits January 18, 2023 10:43

update tests; add collect metrics option

1bfbb17

add failed tests property

98048b4

add test for multiple stack failed

2dcf63f

change path on result

3ceb7c1

Merge branch 'master' into parser_machine_logs

95d44e5

Jasguerrero marked this pull request as ready for review January 24, 2023 17:56

Jasguerrero requested a review from christopherfujino January 24, 2023 17:56

christopherfujino reviewed Jan 24, 2023

View reviewed changes

Jasguerrero added 2 commits January 25, 2023 12:31

create factory method

99e5155

throw exception when test file failed to generate

76c36da

christopherfujino reviewed Jan 25, 2023

View reviewed changes

remove catch of file exception

ce05ea7

christopherfujino reviewed Jan 26, 2023

View reviewed changes

christopherfujino approved these changes Jan 26, 2023

View reviewed changes

Jasguerrero added 2 commits January 27, 2023 14:13

handle when no stacktrace on file reporter

99bc022

Merge branch 'master' into parser_machine_logs

e8652ab

Jasguerrero added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 2, 2023

auto-submit bot merged commit d63987f into flutter:master Feb 2, 2023

This was referenced Feb 3, 2023

Roll Flutter from ca0596e41da3 to c5e8757fcb79 (30 revisions) flutter/packages#3159

Closed

Roll Flutter from d27880801435 to c5e8757fcb79 (54 revisions) flutter/plugins#7092

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 3, 2023

d63987f Parser machine logs (flutter/flutter#118707)

a93bca1

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Feb 3, 2023

d63987f Parser machine logs (flutter/flutter#118707)

124149b

engine-flutter-autoroll mentioned this pull request Feb 3, 2023

Roll Flutter from ca0596e41da3 to 7177c413a9e5 (31 revisions) flutter/packages#3160

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 3, 2023

d63987f Parser machine logs (flutter/flutter#118707)

2163662

engine-flutter-autoroll mentioned this pull request Feb 3, 2023

Roll Flutter from ca0596e41da3 to 96c8c6974186 (32 revisions) flutter/packages#3161

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 3, 2023

d63987f Parser machine logs (flutter/flutter#118707)

e15a0c0

engine-flutter-autoroll mentioned this pull request Feb 3, 2023

Roll Flutter from ca0596e41da3 to 00b0d550c684 (33 revisions) flutter/packages#3162

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 3, 2023

d63987f Parser machine logs (flutter/flutter#118707)

bcf2de8

engine-flutter-autoroll mentioned this pull request Feb 3, 2023

Roll Flutter from ca0596e41da3 to b65ae62cf484 (34 revisions) flutter/packages#3163

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 3, 2023

d63987f Parser machine logs (flutter/flutter#118707)

bbeeb84

engine-flutter-autoroll mentioned this pull request Feb 4, 2023

Roll Flutter from ca0596e41da3 to 61f6a0bcd476 (48 revisions) flutter/packages#3165

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 4, 2023

d63987f Parser machine logs (flutter/flutter#118707)

db32de8

engine-flutter-autoroll mentioned this pull request Feb 4, 2023

Roll Flutter from ca0596e41da3 to 61f6a0bcd476 (48 revisions) flutter/packages#3166

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 4, 2023

d63987f Parser machine logs (flutter/flutter#118707)

13561c1

engine-flutter-autoroll mentioned this pull request Feb 5, 2023

Roll Flutter from ca0596e41da3 to b8f5394a5ca6 (52 revisions) flutter/packages#3167

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 5, 2023

d63987f Parser machine logs (flutter/flutter#118707)

2429348

engine-flutter-autoroll mentioned this pull request Feb 5, 2023

Roll Flutter from ca0596e41da3 to b8f5394a5ca6 (52 revisions) flutter/packages#3168

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 5, 2023

d63987f Parser machine logs (flutter/flutter#118707)

3d4a744

engine-flutter-autoroll mentioned this pull request Feb 6, 2023

Roll Flutter from ca0596e41da3 to 3c3c9a1bd98f (55 revisions) flutter/packages#3170

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 6, 2023

d63987f Parser machine logs (flutter/flutter#118707)

d06b6c2

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 6, 2023

d63987f Parser machine logs (flutter/flutter#118707)

c0eb0fe

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 6, 2023

d63987f Parser machine logs (flutter/flutter#118707)

5c7584f

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 6, 2023

d63987f Parser machine logs (flutter/flutter#118707)

67b9a9b

	for(final String metric in metrics.readAsLinesSync()) {
	for (final String metric in metrics.readAsLinesSync()) {

Parser machine logs #118707

Parser machine logs #118707

Uh oh!

Conversation

Jasguerrero commented Jan 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jasguerrero Jan 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christopherfujino Jan 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christopherfujino left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Jasguerrero commented Jan 18, 2023 •

edited

Loading

Jasguerrero Jan 24, 2023 •

edited

Loading

christopherfujino Jan 25, 2023 •

edited

Loading