Skip to content
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

Increased detectArchitecture() timeout #965

Merged
merged 1 commit into from
May 22, 2020
Merged

Increased detectArchitecture() timeout #965

merged 1 commit into from
May 22, 2020

Conversation

GimpArm
Copy link
Contributor

@GimpArm GimpArm commented Apr 29, 2020

The timeout for detectArchitecture() is sometimes too low when devices are on wifi network connections and even sometimes over USB. The command can take up to 3 seconds to execute and return. Currently the timeout is set to 1000 ms and setting it to 5000 ms seems to be a good compromise.

Platforms affected

Android

Motivation and Context

#963

Description

Increased the detectArchitecture() method timeout from 1000 ms to 5000 ms

Testing

Changing the timeout does not affect and unit tests.

Checklist

  • I've run the tests to see all new and existing tests pass
  • I added automated test coverage as appropriate for this change
  • Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
  • If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)
  • I've updated the documentation if necessary

The timeout for detechArchitecture() is sometimes too low when devices are on wifi network connections and even sometimes over USB. The command can take up to 3 seconds to execute and return. Currently the timeout is set to 1000 ms and setting it to 5000 ms seems to be a good compromise.
@codecov-io
Copy link

Codecov Report

Merging #965 into master will decrease coverage by 0.10%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #965      +/-   ##
==========================================
- Coverage   68.11%   68.00%   -0.11%     
==========================================
  Files          21       21              
  Lines        1866     1866              
==========================================
- Hits         1271     1269       -2     
- Misses        595      597       +2     
Impacted Files Coverage Δ
bin/templates/cordova/lib/build.js 31.61% <ø> (ø)
bin/templates/cordova/lib/check_reqs.js 57.60% <0.00%> (-0.93%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e86b211...5e72576. Read the comment docs.

@breautek
Copy link
Contributor

Thank you for your contribution. I'd like to see one other PMC member vote positive before I merge.

Copy link
Member

@timbru31 timbru31 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Timeout increase should be fine IMO

@breautek breautek merged commit 08dc1dd into apache:master May 22, 2020
@breautek
Copy link
Contributor

breautek commented May 22, 2020

Thanks Tim!

And thank you @GimpArm for your contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants