-
Notifications
You must be signed in to change notification settings - Fork 6k
Upload xcresults to LUCI cloud storage #41647
Conversation
Scenario app test failure and result uploaded: #41652 |
Unittest failed and result uploaded: #41929 |
upload scenario app test result fix script
testing/run_tests.py
Outdated
@@ -752,6 +753,12 @@ def run_objc_tests(ios_variant='ios_debug_sim_unopt', test_filter=None): | |||
ios_unit_test_dir = os.path.join( | |||
BUILDROOT_DIR, 'flutter', 'testing', 'ios', 'IosUnitTests' | |||
) | |||
|
|||
result_bundle_temp = tempfile.TemporaryDirectory( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way you use TemporaryDirectory
is with a with
statement to make sure that it gets cleaned up.
testing/run_tests.py
Outdated
@@ -760,13 +767,27 @@ def run_objc_tests(ios_variant='ios_debug_sim_unopt', test_filter=None): | |||
'xcodebuild ' | |||
'-sdk iphonesimulator ' | |||
'-scheme IosUnitTests ' | |||
"-destination name='" + new_simulator_name + "' " | |||
'-resultBundlePath ' + result_bundle_path + " -destination name='" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please follow the formatting of having a flag at the start of each line.
testing/run_tests.py
Outdated
# Upload the xcresult when the tests fail. | ||
luci_test_outputs_path = os.environ.get('FLUTTER_TEST_OUTPUTS_DIR') | ||
xcresult_bundle = os.path.join(result_bundle_temp, 'ios_embedding.xcresult') | ||
print(xcresult_bundle) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stray print statement.
@@ -41,25 +41,57 @@ fi | |||
# Can also be set via Simulator app Device > Rotate Device Automatically | |||
defaults write com.apple.iphonesimulator RotateWindowWhenSignaledByGuest -int 1 | |||
|
|||
cd $SRC_DIR/out/$FLUTTER_ENGINE/scenario_app/Scenarios | |||
SCENARIO_PATH=$SRC_DIR/out/$FLUTTER_ENGINE/scenario_app/Scenarios | |||
cd $SCENARIO_PATH |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd pushd
and pop around here to make sure you don't effect any downstream commands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I'm just realizing you didn't change this cd
, just shifted the code. The push and pop aren't necessary but don't hurt. Thanks for looking into it.
# Zip and upload xcresult to luci. | ||
# First parameter ($1) is the zip output name. | ||
ZIP_AND_UPLOAD_XCRESULT_TO_LUCI () { | ||
# Using absolute directory causes the zip containing all the sub directories. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you clear this up please? You are probably running into the difference between having a trailing slash or not fwiw.
ZIP_AND_UPLOAD_XCRESULT_TO_LUCI () { | ||
# Using absolute directory causes the zip containing all the sub directories. | ||
# So use relative directory (./$RESULT_BUNDLE_FOLDER) instead. | ||
echo $1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the echo or make it more useful by adding a label.
|
||
# Zip and upload xcresult to luci. | ||
# First parameter ($1) is the zip output name. | ||
ZIP_AND_UPLOAD_XCRESULT_TO_LUCI () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make the function name lowercase please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, thanks!
test_command[0] = test_command[0] + ' -only-testing:%s' % test_filter | ||
run_cmd(test_command, cwd=ios_unit_test_dir, shell=True) | ||
|
||
# except: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gaaclarke Just found this is mistakenly commented out, I will create a patch to fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR landed before I can remove the auto_submit label :( I was a split second slower :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, nice job seeing the problem. This wasn't showing up as an error in CI? Do we need another test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wouldn't as I also missed "try"
…126876) flutter/engine@00f20fb...027ca79 2023-05-15 [email protected] Use release_build and os dimension consistently. (flutter/engine#42012) 2023-05-15 [email protected] [Impeller] Add interactive DrawPaint blend test (flutter/engine#42031) 2023-05-15 [email protected] [Impeller] Limit subpass textures and backdrop blurs to the current clip (flutter/engine#42039) 2023-05-15 [email protected] Roll Dart SDK from c302a0252785 to d2b2ac829842 (1 revision) (flutter/engine#42051) 2023-05-15 [email protected] Upload xcresults to LUCI cloud storage (flutter/engine#41647) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
The original PR #41647 forgot to add try-except-raise fixes flutter/flutter#125823 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
…lutter#126876) flutter/engine@00f20fb...027ca79 2023-05-15 [email protected] Use release_build and os dimension consistently. (flutter/engine#42012) 2023-05-15 [email protected] [Impeller] Add interactive DrawPaint blend test (flutter/engine#42031) 2023-05-15 [email protected] [Impeller] Limit subpass textures and backdrop blurs to the current clip (flutter/engine#42039) 2023-05-15 [email protected] Roll Dart SDK from c302a0252785 to d2b2ac829842 (1 revision) (flutter/engine#42051) 2023-05-15 [email protected] Upload xcresults to LUCI cloud storage (flutter/engine#41647) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Taking over from #41644
fixes: flutter/flutter#125823
Steps to verify
Pre-launch Checklist
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.