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.

Sign up for GitHub

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

Jump to bottom

Other target-dir remapping fixes #577

Closed
wants to merge 3 commits into from
Closed

Other target-dir remapping fixes #577

wants to merge 3 commits into from

Conversation

brody4hire
Copy link

@brody4hire brody4hire commented Nov 23, 2018 •
edited
Loading

Use startsWith instead of includes to check for remapping of target-dir that starts with libs or src/main

(Followup to PR #572)

Discovered while reviewing code related to change proposed in PR #576.

I think this is a very rare case that is not covered by the unit tests. I don't think we should delay 7.1.4 hotfix to get this fix in.

I am not sure whether or not it is worth the time to add unit tests to cover this edge case.

Tests added, expected to be a rare edge case

Other target-dir remapping fixes
9bd952f 
Use `startsWith` instead of `includes` to check for remapping of
`target-dir` that starts with `libs` or `src/main`

(Followup to PR apache#572)
@brody4hire brody4hire added the bug label Nov 23, 2018
@brody4hire brody4hire mentioned this pull request Nov 23, 2018
Closed
brody4hire
brody4hire commented Nov 23, 2018
View reviewed changes
bin/templates/cordova/lib/pluginHandlers.js
@@ -303,13 +303,13 @@ function getInstallDestination (obj) {
return path.join(APP_MAIN_PREFIX, 'java', obj.targetDir.substring(4), path.basename(obj.src));
} else if (obj.src.endsWith('.aidl')) {
return path.join(APP_MAIN_PREFIX, 'aidl', obj.targetDir.substring(4), path.basename(obj.src));
} else if (obj.targetDir.includes('libs')) {
} else if (obj.targetDir.startsWith('libs')) {
Copy link
Author

Choose a reason for hiding this comment

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

From PR #576: What if there is a character other than / after libs?

brody4hire
brody4hire commented Nov 23, 2018
View reviewed changes
bin/templates/cordova/lib/pluginHandlers.js
if (obj.src.endsWith('.so')) {
return path.join(APP_MAIN_PREFIX, 'jniLibs', obj.targetDir.substring(5), path.basename(obj.src));
} else {
return path.join('app', obj.targetDir, path.basename(obj.src));
}
} else if (obj.targetDir.includes('src/main')) {
} else if (obj.targetDir.startsWith('src/main')) {
Copy link
Author

Choose a reason for hiding this comment

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

From PR #576: What if there is a character other than / after src/main ?

@brody4hire brody4hire mentioned this pull request Nov 23, 2018
Open
@codecov-io
Copy link

codecov-io commented Nov 25, 2018 •
edited
Loading

Codecov Report

Merging #577 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #577   +/-   ##
=======================================
  Coverage   62.11%   62.11%           
=======================================
  Files          17       17           
  Lines        1985     1985           
  Branches      371      371           
=======================================
  Hits         1233     1233           
  Misses        752      752
Impacted Files Coverage Δ
bin/templates/cordova/lib/pluginHandlers.js 87.13% <100%> (ø) ⬆️

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 ef24341...f79eda4. Read the comment docs.

@Jule-
Copy link
Contributor

Jule- commented Nov 26, 2018 •
edited
Loading

Test#006b is erroneous! It only succeed because the jar lib is called TestLib.jar with Lib inside. It seems that a specific treatment for .jar is needed. For test purpose we should rename it in Test.jar.

cordova-android/spec/unit/pluginHandlers/handlers.spec.js

Lines 113 to 117 in f79eda4

it('Test#006b : should allow installing jar lib file from sources with new app target-dir scheme', function () {
android['source-file'].install(valid_source[2], dummyPluginInfo, dummyProject, {android_studio: true});
expect(copyFileSpy)
.toHaveBeenCalledWith(dummyplugin, 'src/android/TestLib.jar', temp, path.join('app/libs/TestLib.jar'), false);
});

@brody4hire
Copy link
Author

It only succeed because the jar lib is called TestLib.jar with Lib inside.

Not true since startsWith() is case sensitive. I just pushed an update to show that Test#006b succeeds with test1.jar.

@Jule-
Copy link
Contributor

Jule- commented Nov 26, 2018

@brodybits sorry I have read src as targetDir... 😩

@brody4hire
Copy link
Author

brody4hire commented Nov 26, 2018 •
edited
Loading

Closing in favor of the updates proposed in PR #576. The updates proposed in #576 seem to be a better solution for this and other corner cases.

ref: https://softwareengineering.stackexchange.com/questions/125587/what-are-the-difference-between-an-edge-case-a-corner-case-a-base-case-and-a-b

@brody4hire brody4hire closed this Nov 26, 2018
@Jule- Jule- mentioned this pull request Nov 27, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@brody4hire @codecov-io @Jule-