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

refactor: unify installation on devices & emulators #1123

Merged
merged 2 commits into from
Nov 20, 2020

Conversation

raphinesse
Copy link
Contributor

@raphinesse raphinesse commented Nov 17, 2020

Motivation and Context

device.install and emulator.install are virtually identical. The only notable difference is that emulator.install has some retry logic that is supposed to mitigate emulator hangs. The same goes for the unit tests.

Joining these methods reduces the code we have to maintain and makes upcoming refactors easier.

Description

This change replaces device.install and emulator.install with the generic target.install.

The old installation methods did also resolve the passed target specification (by calling {device,emulator}.resolveTarget) if necessary. The new installation method expects the given installation target to be resolved already. This allows for a better separation of concerns.

Testing

  • Our unit tests pass
  • I manually tested the affected platform-centric binaries. They behave like before this change (failing with TypeError: Cannot read property 'apkPaths' of undefined)

@raphinesse raphinesse added this to the 9.0.1 milestone Nov 17, 2020
@codecov-io
Copy link

codecov-io commented Nov 17, 2020

Codecov Report

Merging #1123 (6737629) into master (aa679ea) will decrease coverage by 0.43%.
The diff coverage is 97.05%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1123      +/-   ##
==========================================
- Coverage   71.41%   70.97%   -0.44%     
==========================================
  Files          20       21       +1     
  Lines        1770     1747      -23     
==========================================
- Hits         1264     1240      -24     
- Misses        506      507       +1     
Impacted Files Coverage Δ
bin/templates/cordova/lib/device.js 100.00% <ø> (ø)
bin/templates/cordova/lib/emulator.js 98.84% <ø> (-0.19%) ⬇️
bin/templates/cordova/lib/target.js 96.77% <96.77%> (ø)
bin/templates/cordova/lib/run.js 100.00% <100.00%> (ø)

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 aa679ea...6737629. Read the comment docs.

@raphinesse
Copy link
Contributor Author

This change is actually not independent from #1101. Sorry for the bother. Closing.

@raphinesse raphinesse closed this Nov 17, 2020
@raphinesse raphinesse deleted the unified-installation branch November 17, 2020 08:58
@raphinesse raphinesse restored the unified-installation branch November 17, 2020 12:46
@raphinesse raphinesse reopened this Nov 17, 2020
@raphinesse
Copy link
Contributor Author

OK, I fixed this PR up and it is now independent and ready to review. Sorry for the inconvenience.

raphinesse added a commit to raphinesse/cordova-android that referenced this pull request Nov 17, 2020
raphinesse added a commit to raphinesse/cordova-android that referenced this pull request Nov 17, 2020
This change replaces the almost identical methods `device.install` and
`emulator.install` with the generic `target.install`.
raphinesse added a commit to raphinesse/cordova-android that referenced this pull request Nov 19, 2020
Copy link
Member

@erisu erisu left a comment

Choose a reason for hiding this comment

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

Unified code and change LGTM 👍

@raphinesse raphinesse merged commit bb7d733 into apache:master Nov 20, 2020
@raphinesse raphinesse deleted the unified-installation branch November 20, 2020 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants