Update hardcoded xHarness version and adb call flow #4635
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Mostly functionality changes Add some more logs and try a copilot solution to the hanging. Add some more logs and try a copilot solution to the hanging. Forcefully close the proc.stdout fd. Update some versions. Remove incorrect closing of stdout holder. Update some versions. Fix a few more locations using adb path. Fix some more missed locations. Comment out testing log statements. Reenable output logging. Update some versions. Update some versions. Update versions to match main.
… STDERR to None to hopefully bypass the hang.
…old adb calls. Along with some other smaller cleanup.
…get runs working with minimal or no changes to it.
… to be printed out in the case of failure during memory consumption tests.
There was a problem hiding this comment.
Copilot reviewed 6 out of 9 changed files in this pull request and generated 1 comment.
Files not reviewed (3)
- eng/performance/maui_scenarios_android.proj: Language not supported
- src/scenarios/shared/devicepowerconsumption.py: Evaluated as low risk
- scripts/performance/common.py: Evaluated as low risk
Comments suppressed due to low confidence (1)
src/scenarios/shared/androidhelper.py:21
- [nitpick] The variable name run_split_regex should be consistently renamed throughout the file.
run_split_regex = r":\s(.+)"
e-kharion
approved these changes
Jan 14, 2025
| adb = RunCommand(cmdline, verbose=True) | ||
| adb.run() | ||
| # Try calling xharness with stdout=None and stderr=None to hopefully bypass the hang | ||
| getLogger().info("Clearing xharness stdout and stderr to avoid hang") |
There was a problem hiding this comment.
this pretty much duplicates the code of pre.py. I know it's probably out of scope of this PR, but if there is gonna be an additional one with further refactoring, would it make sense to deduplicate this part?
There was a problem hiding this comment.
+1 especially since it is a tricky problem
ivanpovazan
approved these changes
Jan 14, 2025
| adb = RunCommand(cmdline, verbose=True) | ||
| adb.run() | ||
| # Try calling xharness with stdout=None and stderr=None to hopefully bypass the hang | ||
| getLogger().info("Clearing xharness stdout and stderr to avoid hang") |
There was a problem hiding this comment.
+1 especially since it is a tricky problem
matouskozak
approved these changes
Jan 14, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Updated xHarness version we have hardcoded for android runs and minor cleanup of src/scenarios/shared/androidinstrumentation.py by removing a layer from try except finally. The version update was accomplished primarily by updating all previous direct adb arguments with xharness_adb arguments (added to common.py for easy reuse and updating) and implementing a hang workaround where xharness hangs if the first call to xharness adb pipes to STDOUT.
This will solve problems 1 and 2 from #4574, using an outdated version and having outdated command conventions. More work will need to be done to standardize the iOS and android xharness versions to be the same and have them auto-update using maestro.
Successful Pixel test run: https://dev.azure.com/dnceng/internal/_build/results?buildId=2619524&view=logs&j=efa3ffcd-91e9-5b69-9db7-650958b3131c&t=a635f724-5afe-5774-89bd-de12fd2d4e6e