change how we parse simulator output to incorporate try bots by nturgut · Pull Request #18 · flutter/web_installers

nturgut · 2020-06-24T22:23:36Z

The try bots output for xcrun simctl list was slightly different then my local and the prod bots.

Therefore the code that I wrote for parsing the id was failing.

Locally tested with output from link: https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket.appspot.com/8876584155254712000/+/steps/felt_ios-safari_test/0/stdout

christopherfujino · 2020-06-24T22:37:28Z

packages/simulators/lib/simulator_manager.dart

    final String restOfTheOutput = simulatorsList
        .substring(indexOfVersionListStart + simulatorVersion.length);
-    final int indexOfNextVersion = restOfTheOutput.indexOf('--');
+    int indexOfNextVersion = restOfTheOutput.indexOf('--');


can we do a more specific regex, rather than relying on --? I worry that a future change to this output could lead to a hard to spot regression.

Hmm, never mind...I see that would be difficult...

christopherfujino

I will approve this to unblock you. I think the long-term solution is to pass the --json flag to simctl. For reference: https://github.com/flutter/flutter/blob/master/packages/flutter_tools/lib/src/ios/simulators.dart#L89

packages/simulators/lib/simulator_manager.dart

yjbanov · 2020-06-24T22:39:31Z

packages/simulators/lib/simulator_manager.dart

    final int indexOfVersionListStart =
        simulatorsList.indexOf(simulatorVersion);
    final String restOfTheOutput = simulatorsList
        .substring(indexOfVersionListStart + simulatorVersion.length);


What if indexOfVersionListStart == -1?

This will also fail an exception like now. This code relies on the current formatting very much: "--" or "==" kills all the code. I think @christopherfujino suggestion would be the best change.

yjbanov · 2020-06-24T22:44:18Z

packages/simulators/lib/simulator_manager.dart

+    //     iPad Pro (11-inch) (E0B89C9C-6200-495C-B18B-0078CCAAC688) (Shutdown)
+    //     iPad Pro (12.9-inch) (3rd generation) (DB3EB7A8-C4D2-4F86-AFC1-D652FB0579E8) (Shutdown)
+    //     iPad Air (3rd generation) (9237DCD8-8F0E-40A6-96DF-B33C915AFE1B) (Shutdown)
+    // == Device Pairs ==


I think this implementation can be simplified if it first split the output line by line, then inspected each line. There are only two interesting line formats:

-- OS MAJOR.MINOR --, such as -- iOS 13.0

DEVICE (HASH) (STATE), sush as iPhone 8 (C142C9F5-C26E-4EB5-A2B8-915D5BD62FA5) (Shutdown)

A single line-by-line for/in loop can detect both OS version and the requested device.

thanks a lot for the suggestion, I think I'll used --json tag and will get rid of this parsing all together :)

yjbanov · 2020-06-24T22:47:18Z

packages/simulators/lib/simulator_manager.dart

    //   iPhone 11 Pro (D8074C8B-35A5-4DA5-9AB2-4CE738A5E5FC) (Shutdown)
+    // Example output 2 (from Mac Web Engine try bots):
+    // == Devices ==
+    // -- iOS 13.0 --


Is this example demonstrating the need for the new code? If so, then I'm not sure how it does that. restOfTheOutput.indexOf('--') is >0 (because the string does contain --), so the new code won't execute 🤔

in one of them the version was finishing with '--' now it finishes with "==". let me write an explanation, but as mentioned in the earlier comment my next PR would be just removing these.

I have local test for these things btw which I don't merge let me also send it!

nturgut · 2020-06-24T23:04:20Z

I will approve this to unblock you. I think the long-term solution is to pass the --json flag to simctl. For reference: https://github.com/flutter/flutter/blob/master/packages/flutter_tools/lib/src/ios/simulators.dart#L89

I think this is a very good solution! Thanks a lot, I was under the impression that this parsing will cause me many more bugs in the future.

I feel the ultimate savior would be (initially suggested by @yjbanov ) triggering CI from this bot. I was reading the document @godofredoc shared with me but I still found it a little hard to add LUCI to presubmit. Do you have some example PRs that can help to add LUCI to web_installers.

christopherfujino · 2020-06-24T23:19:34Z

I feel the ultimate savior would be (initially suggested by @yjbanov ) triggering CI from this bot. I was reading the document @godofredoc shared with me but I still found it a little hard to add LUCI to presubmit. Do you have some example PRs that can help to add LUCI to web_installers.

Sorry, I'm not even sure what this repo is :P. Would you be triggering existing recipes or adding a new one specifically to exercise changes to this logic?

nturgut · 2020-06-24T23:23:20Z

I feel the ultimate savior would be (initially suggested by @yjbanov ) triggering CI from this bot. I was reading the document @godofredoc shared with me but I still found it a little hard to add LUCI to presubmit. Do you have some example PRs that can help to add LUCI to web_installers.

Sorry, I'm not even sure what this repo is :P. Would you be triggering existing recipes or adding a new one specifically to exercise changes to this logic?

Sorry I used it for repository :) I meant flutter/web_installers repository

I would like to write a small recipe for flutter/web_installers repository. I want to run it Mac Web Engine try bots when a PR is send out to this repo. I read the documentation but it would be best if there are example PRs.

christopherfujino · 2020-06-24T23:25:14Z

I would like to write a small recipe for flutter/web_installers repository. I want to run it Mac Web Engine try bots when a PR is send out to this repo. I read the documentation but it would be best if there are example PRs.

I've never added LUCI checks to a GitHub repo. Maybe @Piinks could link some sample PRs? I think she did this for the SkiaGold check.

godofredoc · 2020-06-24T23:26:08Z

I feel the ultimate savior would be (initially suggested by @yjbanov ) triggering CI from this bot. I was reading the document @godofredoc shared with me but I still found it a little hard to add LUCI to presubmit. Do you have some example PRs that can help to add LUCI to web_installers.

Sorry, I'm not even sure what this repo is :P. Would you be triggering existing recipes or adding a new one specifically to exercise changes to this logic?

Please let me know where are you stuck in the process of adding luci support and I'll improve the docs.

godofredoc · 2020-06-25T06:16:38Z

would be best if there are example PRs.
@digiter added the packages repo recently maybe we can use his PRs you may want to start updating the webhook configurations for the repo you'll need the help of an admin for that.

change how we parse simulator output to incorporate try bots

5dce9dd

nturgut requested a review from yjbanov June 24, 2020 22:23

christopherfujino reviewed Jun 24, 2020

View reviewed changes

christopherfujino approved these changes Jun 24, 2020

View reviewed changes

yjbanov approved these changes Jun 24, 2020

View reviewed changes

addressing reviewer comments

8d55864

make the test version same with wbe_drivers package

6b79214

nturgut merged commit 1da6bb8 into flutter:master Jun 24, 2020

nturgut mentioned this pull request Jun 25, 2020

ios-safari tests are failing on Mac Try Bots flutter/flutter#60251

Closed

Conversation

nturgut commented Jun 24, 2020

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!

Uh oh!

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!

nturgut commented Jun 24, 2020

Uh oh!

christopherfujino commented Jun 24, 2020

Uh oh!

nturgut commented Jun 24, 2020

Uh oh!

christopherfujino commented Jun 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

godofredoc commented Jun 24, 2020

Uh oh!

godofredoc commented Jun 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

christopherfujino commented Jun 24, 2020 •

edited

Loading