-
Notifications
You must be signed in to change notification settings - Fork 115
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.
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
Fix test on emulator workflow failures #734
Conversation
❌ Integration test FAILEDRequested by @sunmou99 on commit 9efb188
Add flaky tests to go/fpl-cpp-flake-tracker |
@@ -59,12 +59,12 @@ | |||
Device Information is stored in TEST_DEVICES in print_matrix_configuration.py | |||
Example: | |||
sdk id "system-images;android-29;google_apis;x86": | |||
--android_sdk "system-images;android-29;google_apis;x86" --build_tools_version "28.0.3" | |||
--android_sdk "system-images;android-29;google_apis;x86" --build_tools_version "29.0.2" |
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.
Is there a need to update the version here since it is just an example?
Similarly to line 67
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.
No need, but I feel there is not harm too.
if retry_time > 1: | ||
try: | ||
subprocess.run(args, shell=shell, check=check, timeout=timeout) | ||
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.
We should log the exception here so that we know what is triggering the retry.
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.
subprocess.run
itself print out logs and exceptions.
log example: https://github.com/firebase/firebase-cpp-sdk/runs/4145793172?check_suite_focus=true#step:9:78
else: | ||
time.sleep(45) | ||
|
||
def _run_with_retry(args, shell=False, check=True, timeout=_CMD_TIMEOUT, retry_time=_TEST_RETRY, device=_DEVICE_NONE, type=_RESET_TYPE_REBOOT): |
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.
We should log when all the retry attempts failed.
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.
Thanks for the suggestion. I just added log in another PR (due to branch dependency): https://github.com/firebase/firebase-cpp-sdk/pull/733/files#diff-51ade566b474eb1faa5dfc7cfdcc3d943f850740c053a79ba0e4faa9f6f89315R679
|
||
args = ["sdkmanager", "--update"] | ||
logging.info("Update all installed packages: %s", " ".join(args)) | ||
subprocess.run(args=args, check=True) | ||
_run_with_retry(args, check=False) |
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.
Why changing check from True to False?
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.
This is a non-critical step.
check=True
only when this step block next steps.
@@ -129,7 +137,7 @@ | |||
"android_sdk", "system-images;android-29;google_apis;x86", | |||
"See module docstring for details on how to set and get this id.") | |||
flags.DEFINE_string( | |||
"build_tools_version", "28.0.3", | |||
"build_tools_version", "29.0.2", |
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.
Isn't this value in build.gradle
? Is this the right place to update the config value like this?
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.
We are using tools under $ANDROID_HOME/build-tools/${build_tools_version}
* Fix test on emulator workflow failures (#734) * If simulator install ios app failed, reset simulator and try again (#733) * Trigger workflow move github api cod to github.py (#746) * Fix a data race that could manifest as null pointer dereference in FutureBase::Release() (#747) * Cancel callbacks for messaging (#745) * Cancel callbacks for messaging util::Terminate is referenced counted som when there ar more APIs than messaging active the callbacks will not be canceled until later and still cause a NULL ref due to the FutureData being destroyed now. * Cancel callback earlier * Update readme * Remove "Android" tag from the release notes entry for #747 (#749) * Remove calls to LogInfo, LogError, LogDebug during obj-c +load. (#706) * Remove calls to LogInfo, LogError, LogDebug during obj-c +load. This could be causing an issue in C++ as global class constructors have not yet been run. * Add Objective-C/C++ and Java to code formatter script; format those files. (#755) * Allow format_code to format .m/.mm files; clang-format already knows how. * Run format_code.py on all objective-c/objective-c++ files. * Add Java file extensions to format_code.py * Format all Java source files. * Remove check for objc header, as they are now supported. * Format objective-c .h files. * Don't let lint comment on line length any more; code formatting will report that. * Messaging crash during initialization (#760) * Messaging crash during initialization * Update readme * Don't redeclare inherited state in CredentialsProviderDesktop (#731) * Reduce disk space usage when packaging the built SDK (#763) Remove intermediate build files during desktop packaging step. This should reduce the disk space usage, as those files (*.o and *.obj) are not required when merging libraries. * Workaround for Linux x86 build: downgrade libraries on GitHub runners (#764) When installing 32-bit Linux dependencies on GitHub runners, downgrade libpcre2-8-0 to an earlier version to ensure compatibility with the i386 version of the package. This is something that should be fixed in a subsequent Ubuntu release and so is a temporary workaround. This also adds checks to the various prerequisite commands run by build_desktop.py, which was previously just silently ignoring errors (making this much harder to track down). Now it will error out as soon as a command fails. Co-authored-by: Mou Sun <69009538+sunmou99@users.noreply.github.com> Co-authored-by: Denver Coneybeare <dconeybe@google.com> Co-authored-by: Tobias Barendt <tobias@robotsquid.com> Co-authored-by: Jon Simantov <jsimantov@google.com> Co-authored-by: Sebastian Schmidt <mrschmidt@google.com>
Description
Test on Emualtor sometimes failed when install Android app. https://github.com/firebase/firebase-cpp-sdk/runs/4078767834?check_suite_focus=true
This article suggests delete the old emulator and create a new one :
https://medium.com/@aedwin905/solved-failure-calling-service-package-broken-pipe-32-3b860c7e04bb
Also saw some errors when downloading emulator, uninstalling apps.
Thus, I made the following changes:
Testing
See the testing result in the comment
Type of Change
Place an
x
the applicable box: