Skip to content

Conversation

@AnimaSA
Copy link
Contributor

@AnimaSA AnimaSA commented Mar 20, 2019

Summary:

Adapted the changes from here that was opened but never merged. This PR changes the behavior the run-android to use the "launch package" (the final application id built by gradle) instead of statically looking at package in app/src/main/AndroidManifest.xml. It accomplishes this via scraping the merged manifest output after the app is built, so any changes applied by gradle are reflected.

Since the internal folder structure is the same, it still uses the original value the launch activity. The final intent now looks like:
Starting: Intent { cmp=com.company.any.other.gradle.changes/com.company.MainActivity }
Which is very useful when working with multiple build types (.debug appended at the end is a very common case for debug builds) and build flavors.

Test Plan:

Tested locally in my build process for the app my company is working on.

Copy link
Member

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!
CC @krizzu

Copy link
Member

@krizzu krizzu left a comment

Choose a reason for hiding this comment

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

Looks good to me, great work 👍
I've left few comments for you, let me know what you think.

Also, it would be great to have test for new functionality.

@AnimaSA
Copy link
Contributor Author

AnimaSA commented Mar 21, 2019

Just pushed new commits addressing your feedback. I'm new to writing test cases so hopefully it should suffice.

Thanks!

@thymikee thymikee requested review from Esemesek and Salakar March 22, 2019 07:34
@thymikee
Copy link
Member

cc @dulmandakh & @bartolkaruza for review

@thymikee thymikee changed the title run-android respects applicationId from build.gradle when running app feat: run-android respects applicationId from build.gradle when running app Mar 22, 2019
Copy link

@bartolkaruza bartolkaruza left a comment

Choose a reason for hiding this comment

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

Apart from two changes for the test, it looks good to me.

);
}
});

Choose a reason for hiding this comment

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

please add a test case for flavor.buildType as well as just buildType

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a test for a flavored build. Let me know if it works.


fs.readFileSync = jest.fn(filename => {
switch (filename) {
case 'app/build.gradle':

Choose a reason for hiding this comment

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

this is throwing for me when I run the tests

(node:8120) UnhandledPromiseRejectionWarning: Error: ENOENT: no such file or directory, open 'app/build.gradle'
    at Object.openSync (fs.js:439:3)
    at Object.readFileSync (fs.js:344:35)
    at findBuildTypes (/Users/bartolkaruza/dev/proj/oss/react-native-cli/packages/cli/src/commands/runAndroid/getLaunchPackageName.js:58:33)
    at splitVariant (/Users/bartolkaruza/dev/proj/oss/react-native-cli/packages/cli/src/commands/runAndroid/getLaunchPackageName.js:96:22)
    at getManifestFile (/Users/bartolkaruza/dev/proj/oss/react-native-cli/packages/cli/src/commands/runAndroid/getLaunchPackageName.js:136:7)
    at getLaunchPackageName (/Users/bartolkaruza/dev/proj/oss/react-native-cli/packages/cli/src/commands/runAndroid/getLaunchPackageName.js:155:24)
    at runOnAllDevices (/Users/bartolkaruza/dev/proj/oss/react-native-cli/packages/cli/src/commands/runAndroid/runOnAllDevices.js:83:99)
    at Object.it (/Users/bartolkaruza/dev/proj/oss/react-native-cli/packages/cli/src/commands/runAndroid/__tests__/runOnAllDevices.test.js:29:34)
    at Object.asyncJestTest (/Users/bartolkaruza/dev/proj/oss/react-native-cli/node_modules/jest-jasmine2/build/jasmineAsyncInstall.js:100:37)
    at resolve (/Users/bartolkaruza/dev/proj/oss/react-native-cli/node_modules/jest-jasmine2/build/queueRunner.js:41:12)

Choose a reason for hiding this comment

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

The failing promises are on other tests than the current one, so try running all tests for cli to reproduce. (yarn test) Output should be clean success for all tests, as it is on current master.

Copy link
Contributor Author

@AnimaSA AnimaSA Apr 2, 2019

Choose a reason for hiding this comment

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

I've pushed a commit that updates the tests that indirectly call getLaunchPackage to also mock the fs there, as that was causing the issue. However, it appears there's a Windows specific issue for the getDependencyConfig test: It's comparing a *nix path in the snapshot to a Windows generated one, which will always fail.

Here is the output under Windows/Powershell

PS ...> yarn test
yarn run v1.13.0
$ jest
 PASS  packages/cli/src/tools/__tests__/findPlugins-test.js
 FAIL  packages/cli/src/commands/link/__tests__/getDependencyConfig-test.js
  ● getDependencyConfig › should return an array of dependencies' config

    expect(value).toMatchSnapshot()

    Received value does not match stored snapshot "getDependencyConfig should return an array of dependencies' config 1".

    - Snapshot
    + Received

    @@ -9,7 +9,7 @@
            "sampleiOSKey": "",
          },
        },
        "name": "react-native-windows",
        "params": Array [],
    -   "path": "/root/node_modules/react-native-windows",
    +   "path": "\\root\\node_modules\\react-native-windows",
      }

      35 |     );
      36 | 
    > 37 |     expect(dependencies).toMatchSnapshot();
         |                          ^
      38 |   });
      39 | 
      40 |   it('should throw on invalid react-native dependency', () => {

      at Object.toMatchSnapshot (src/commands/link/__tests__/getDependencyConfig-test.js:37:26)

  ● getDependencyConfig › should throw on invalid react-native dependency

    expect(received).toThrowError()

    Received function did not throw

      41 |     expect(() =>
      42 |       getDependencyConfig({root: '/root'}, platforms, 'abcd'),
    > 43 |     ).toThrowError();
         |       ^
      44 |   });
      45 | });
      46 | 

      at Object.toThrowError (src/commands/link/__tests__/getDependencyConfig-test.js:43:7)

 › 1 snapshot failed.
 PASS  packages/cli/src/commands/upgrade/__tests__/upgrade.test.js
 PASS  packages/cli/src/commands/runAndroid/__tests__/runOnAllDevices.test.js
 PASS  packages/cli/src/tools/__tests__/android/getProjectConfig-test.js
 PASS  packages/cli/src/tools/__tests__/ios/getProjectConfig-test.js
 PASS  packages/cli/src/commands/link/__tests__/link-test.js
 PASS  packages/cli/src/commands/link/__tests__/ios/createGroup-test.js
 PASS  packages/cli/src/commands/link/__tests__/ios/removeSharedLibrary-test.js
 PASS  packages/cli/src/commands/info/__tests__/info.test.js
 PASS  packages/cli/src/tools/__tests__/ios/findProject-test.js
 PASS  packages/cli/src/commands/link/__tests__/ios/getPlist-test.js
 PASS  packages/cli/src/commands/link/__tests__/ios/addSharedLibraries-test.js
 PASS  packages/cli/src/commands/link/__tests__/ios/writePlist-test.js
 PASS  packages/cli/src/commands/link/__tests__/ios/mapHeaderSearchPaths-test.js
 PASS  packages/cli/src/commands/link/__tests__/ios/addProjectToLibraries-test.js
 PASS  packages/cli/src/commands/link/__tests__/ios/getPlistPath-test.js
 PASS  packages/cli/src/commands/link/__tests__/ios/removeProjectFromProject-test.js
 PASS  packages/cli/src/commands/link/__tests__/pods/findPodTargetLine-test.js
 PASS  packages/cli/src/commands/link/__tests__/ios/getGroup-test.js
 PASS  packages/cli/src/commands/link/__tests__/ios/isInstalled-test.js
 PASS  packages/cli/src/tools/__tests__/android/getDependencyConfig-test.js
 PASS  packages/cli/src/commands/link/__tests__/ios/getBuildProperty-test.js
 PASS  packages/cli/src/commands/server/debugger-ui/__tests__/DeltaPatcher-test.js
 PASS  packages/cli/src/tools/__tests__/findSymlinkedModules-test.js
 PASS  packages/cli/src/commands/link/__tests__/ios/hasLibraryImported-test.js
 PASS  packages/cli/src/tools/__tests__/android/readManifest-test.js
 PASS  packages/cli/src/tools/__tests__/android/findAndroidAppFolder-test.js
 PASS  packages/cli/src/commands/link/__tests__/ios/addFileToProject-test.js
 PASS  packages/cli/src/commands/link/__tests__/android/normalizeProjectName-test.js
 PASS  packages/cli/src/commands/link/__tests__/ios/getTargets-test.js
 PASS  packages/cli/src/commands/link/__tests__/ios/removeProjectFromLibraries-test.js
 PASS  packages/cli/src/tools/__tests__/ios/findPodspecName-test.js
 PASS  packages/cli/src/commands/link/__tests__/pods/isInstalled-test.js
 PASS  packages/cli/src/tools/__tests__/PackageManager-test.js
 PASS  packages/cli/src/commands/runAndroid/__tests__/getLaunchPackageName.test.js
 PASS  packages/cli/src/tools/__tests__/makeCommand-test.js
 PASS  packages/cli/src/tools/__tests__/findAssets-test.js
 PASS  packages/cli/src/commands/link/__tests__/android/makeStringsPatch-test.js
 PASS  packages/cli/src/tools/__tests__/android/findPackageClassName-test.js
 PASS  packages/cli/src/commands/link/__tests__/groupFilesByType-test.js
 PASS  packages/cli/src/tools/__tests__/android/findManifest-test.js
 PASS  packages/cli/src/commands/link/__tests__/ios/getHeaderSearchPath-test.js
 PASS  packages/cli/src/commands/link/__tests__/android/applyPatch-test.js
 PASS  packages/cli/src/commands/runIOS/__tests__/findMatchingSimulator-test.js
 PASS  packages/cli/src/commands/link/__tests__/promiseWaterfall-test.js
 PASS  packages/cli/src/commands/link/__tests__/android/makeSettingsPatch-test.js
 PASS  packages/cli/src/commands/bundle/__tests__/getAssetDestPathAndroid-test.js
 PASS  packages/cli/src/commands/link/__tests__/android/isInstalled-test.js
 PASS  packages/cli/src/commands/link/__tests__/ios/getHeadersInFolder-test.js
 PASS  packages/cli/src/commands/link/__tests__/android/makeBuildPatch-test.js
 PASS  packages/cli/src/tools/__tests__/ios/findPodfilePath-test.js
 PASS  packages/cli/src/commands/link/__tests__/android/makePackagePatch-test.js
 PASS  packages/cli/src/commands/bundle/__tests__/getAssetDestPathIOS-test.js
 PASS  packages/cli/src/commands/runIOS/__tests__/findXcodeProject-test.js
 PASS  packages/cli/src/commands/link/__tests__/pods/findMarkedLinesInPodfile-test.js
 PASS  packages/cli/src/commands/bundle/__tests__/filterPlatformAssetScales-test.js
 PASS  packages/cli/src/commands/link/__tests__/getProjectDependencies-test.js
 PASS  packages/cli/src/commands/link/__tests__/android/makeImportPatch-test.js
 PASS  packages/cli/src/commands/link/__tests__/pods/findLineToAddPod-test.js
 PASS  packages/cli/src/commands/runIOS/__tests__/parseIOSDevicesList-test.js
 PASS  packages/cli/src/commands/link/__tests__/pods/removePodEntry-test.js
 PASS  e2e/__tests__/install.test.js (7.307s)
 PASS  e2e/__tests__/uninstall.test.js (8.975s)

Summary of all failing tests
 FAIL  packages/cli/src/commands/link/__tests__/getDependencyConfig-test.js
  ● getDependencyConfig › should return an array of dependencies' config

    expect(value).toMatchSnapshot()

Snapshot Summary
 › 1 snapshot failed from 1 test suite. Inspect your code changes or run `yarn test -u` to update them.

Test Suites: 1 failed, 63 passed, 64 total
Tests:       2 failed, 2 todo, 225 passed, 229 total
Snapshots:   1 failed, 7 passed, 8 total
Time:        10.106s
Ran all test suites in 2 projects.

And the same test, ran under WSL/Ubuntu:

AnimaSA@DESKTOP-D0NNEGG /m/c/U/C/D/P/react-native-cli> yarn test
yarn run v1.13.0
$ jest
 PASS  packages/cli/src/commands/server/debugger-ui/__tests__/DeltaPatcher-test.js
 PASS  packages/cli/src/tools/__tests__/findPlugins-test.js
 PASS  packages/cli/src/tools/__tests__/findSymlinkedModules-test.js
 PASS  packages/cli/src/commands/runIOS/__tests__/findMatchingSimulator-test.js
 PASS  packages/cli/src/tools/__tests__/PackageManager-test.js
 PASS  packages/cli/src/commands/link/__tests__/android/makeSettingsPatch-test.js
 PASS  packages/cli/src/commands/link/__tests__/ios/getHeaderSearchPath-test.js
 PASS  packages/cli/src/commands/runAndroid/__tests__/runOnAllDevices.test.js
 PASS  packages/cli/src/tools/__tests__/ios/findProject-test.js
 PASS  packages/cli/src/commands/link/__tests__/android/makeBuildPatch-test.js
 PASS  packages/cli/src/commands/upgrade/__tests__/upgrade.test.js
 PASS  packages/cli/src/commands/bundle/__tests__/getAssetDestPathAndroid-test.js
 PASS  packages/cli/src/commands/link/__tests__/pods/removePodEntry-test.js
 PASS  packages/cli/src/commands/link/__tests__/pods/findLineToAddPod-test.js
 PASS  packages/cli/src/commands/link/__tests__/ios/writePlist-test.js
 PASS  packages/cli/src/tools/__tests__/android/getProjectConfig-test.js
 PASS  packages/cli/src/tools/__tests__/android/getDependencyConfig-test.js
 PASS  packages/cli/src/commands/link/__tests__/ios/createGroup-test.js
 PASS  packages/cli/src/commands/info/__tests__/info.test.js
 PASS  e2e/__tests__/uninstall.test.js (7.324s)
 PASS  e2e/__tests__/install.test.js (8.516s)
 PASS  packages/cli/src/tools/__tests__/PackageManager-test.js
 PASS  packages/cli/src/tools/__tests__/findPlugins-test.js
 PASS  packages/cli/src/tools/__tests__/findSymlinkedModules-test.js
 PASS  packages/cli/src/commands/server/debugger-ui/__tests__/DeltaPatcher-test.js
 PASS  packages/cli/src/commands/runIOS/__tests__/findMatchingSimulator-test.js
 PASS  packages/cli/src/tools/__tests__/android/getProjectConfig-test.js
 PASS  packages/cli/src/commands/runAndroid/__tests__/runOnAllDevices.test.js
 PASS  packages/cli/src/commands/link/__tests__/ios/addSharedLibraries-test.js
 PASS  packages/cli/src/commands/link/__tests__/ios/writePlist-test.js
 PASS  packages/cli/src/commands/link/__tests__/ios/isInstalled-test.js
 PASS  packages/cli/src/tools/__tests__/ios/findProject-test.js
 PASS  packages/cli/src/commands/link/__tests__/ios/createGroup-test.js
 PASS  packages/cli/src/commands/link/__tests__/link-test.js
 PASS  packages/cli/src/commands/upgrade/__tests__/upgrade.test.js
 PASS  packages/cli/src/commands/link/__tests__/android/makeSettingsPatch-test.js
 PASS  packages/cli/src/commands/link/__tests__/ios/hasLibraryImported-test.js
 PASS  packages/cli/src/commands/link/__tests__/ios/removeProjectFromProject-test.js
 PASS  packages/cli/src/tools/__tests__/android/getDependencyConfig-test.js
 PASS  packages/cli/src/tools/__tests__/ios/getProjectConfig-test.js
 PASS  packages/cli/src/tools/__tests__/ios/findPodfilePath-test.js
 PASS  packages/cli/src/commands/bundle/__tests__/getAssetDestPathAndroid-test.js
 PASS  packages/cli/src/commands/link/__tests__/ios/mapHeaderSearchPaths-test.js
 PASS  packages/cli/src/commands/link/__tests__/getDependencyConfig-test.js
 PASS  packages/cli/src/commands/link/__tests__/ios/addFileToProject-test.js
 PASS  packages/cli/src/commands/link/__tests__/ios/removeProjectFromLibraries-test.js
 PASS  packages/cli/src/commands/link/__tests__/ios/getGroup-test.js
 PASS  packages/cli/src/commands/link/__tests__/ios/removeSharedLibrary-test.js
 PASS  packages/cli/src/tools/__tests__/android/findPackageClassName-test.js
 PASS  packages/cli/src/commands/link/__tests__/ios/addProjectToLibraries-test.js
 PASS  packages/cli/src/commands/link/__tests__/ios/getHeaderSearchPath-test.js
 PASS  packages/cli/src/commands/link/__tests__/ios/getTargets-test.js
 PASS  packages/cli/src/commands/link/__tests__/android/makeBuildPatch-test.js
 PASS  packages/cli/src/tools/__tests__/ios/findPodspecName-test.js
 PASS  packages/cli/src/commands/link/__tests__/ios/getPlist-test.js
 PASS  packages/cli/src/commands/link/__tests__/ios/getPlistPath-test.js
 PASS  packages/cli/src/tools/__tests__/findAssets-test.js
 PASS  packages/cli/src/tools/__tests__/android/readManifest-test.js
 PASS  packages/cli/src/commands/link/__tests__/pods/findLineToAddPod-test.js
 PASS  packages/cli/src/commands/link/__tests__/ios/getBuildProperty-test.js
 PASS  packages/cli/src/commands/link/__tests__/android/makePackagePatch-test.js
 PASS  packages/cli/src/commands/link/__tests__/pods/removePodEntry-test.js
 PASS  packages/cli/src/commands/link/__tests__/groupFilesByType-test.js
 PASS  packages/cli/src/commands/bundle/__tests__/filterPlatformAssetScales-test.js
 PASS  packages/cli/src/commands/runIOS/__tests__/parseIOSDevicesList-test.js
 PASS  packages/cli/src/commands/bundle/__tests__/getAssetDestPathIOS-test.js
 PASS  packages/cli/src/commands/runIOS/__tests__/findXcodeProject-test.js
 PASS  packages/cli/src/commands/link/__tests__/pods/isInstalled-test.js
 PASS  packages/cli/src/commands/link/__tests__/promiseWaterfall-test.js
 PASS  packages/cli/src/commands/link/__tests__/pods/findPodTargetLine-test.js
 PASS  packages/cli/src/tools/__tests__/android/findAndroidAppFolder-test.js
 PASS  packages/cli/src/commands/link/__tests__/android/isInstalled-test.js
 PASS  packages/cli/src/commands/link/__tests__/android/normalizeProjectName-test.js
 PASS  packages/cli/src/tools/__tests__/android/findManifest-test.js
 PASS  packages/cli/src/commands/link/__tests__/ios/getHeadersInFolder-test.js
 PASS  packages/cli/src/commands/link/__tests__/android/makeImportPatch-test.js
 PASS  packages/cli/src/commands/link/__tests__/pods/findMarkedLinesInPodfile-test.js
 PASS  packages/cli/src/commands/link/__tests__/getProjectDependencies-test.js
 PASS  packages/cli/src/commands/link/__tests__/android/makeStringsPatch-test.js
 PASS  packages/cli/src/tools/__tests__/makeCommand-test.js
 PASS  packages/cli/src/commands/link/__tests__/android/applyPatch-test.js
 PASS  packages/cli/src/commands/runAndroid/__tests__/getLaunchPackageName.test.js

Test Suites: 61 passed, 61 total
Tests:       1 todo, 220 passed, 221 total
Snapshots:   8 passed, 8 total
Time:        7.322s
Ran all test suites.
Done in 8.67s.

@grabbou
Copy link
Member

grabbou commented Apr 2, 2019

Hey @AnimaSA, thanks for this great PR! Any chance you'd be able to look into the comments in the nearest future?

@AnimaSA
Copy link
Contributor Author

AnimaSA commented Apr 2, 2019

Yup, I'll be updating it soon!

@grabbou
Copy link
Member

grabbou commented Apr 10, 2019

@AnimaSA, run-android is now under a different package. Shouldn't be too hard to rebase. Let me know if you need any help.

@AnimaSA AnimaSA force-pushed the feature/bugfix-run-android branch from 8a7004a to 4009acb Compare April 11, 2019 16:44
@AnimaSA
Copy link
Contributor Author

AnimaSA commented Apr 11, 2019

Rebased on latest master and updated PR.

function getManifestFile(variant) {
// get the path to the correct manifest file to find the correct package name to be used while
// starting the app
const gradleFilePath = path.join('app', 'build.gradle');
Copy link
Member

Choose a reason for hiding this comment

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

Something to do in the future. We have a location of buildGradle available in the config. We could use that instead of hardcoded path.

But we don't use it yet in runAndroid, so no need to think about it at this point.

@thymikee thymikee closed this Apr 17, 2019
@thymikee thymikee reopened this Apr 17, 2019
@thymikee
Copy link
Member

Closed and reopened the PR to re-trigger CI as it didn't click, likely after chaning the repo name.

Build system may alter application id for build and flavor purposes. This commit alters the system to not need a packageNameWithSuffix, but instead pull the launch package name directly from the merged manifest and use that to launch it on the device.
Also fixed some faulty RegExps as a result of testing.
@thymikee thymikee force-pushed the feature/bugfix-run-android branch from 4009acb to fdcd58e Compare April 17, 2019 12:35
@thymikee thymikee requested a review from grabbou April 17, 2019 13:00
@grabbou
Copy link
Member

grabbou commented Apr 17, 2019

Unit tests are failing, just in case someone missed it!

@thymikee
Copy link
Member

It's this flaky test. Unrelated

@thymikee thymikee requested a review from krizzu May 6, 2019 17:29
* origin/master:
  fix: refactor init for less duplication; fix handling versioned templates (react-native-community#364)
  docs: add `maintainers` section (react-native-community#369)
  feat: copy all files in `init` through streams (react-native-community#363)
  feat: init from tarball (react-native-community#362)
  fix: init from url (react-native-community#361)
@grabbou
Copy link
Member

grabbou commented Jan 30, 2020

It's really unfortunate that this PR has been open for so long! But... it's time to finally merge it! I am going to resolve the conflicts myself (and learn about the changes on that occasion) and I am looking forward to shipping that in a few minutes!

@grabbou
Copy link
Member

grabbou commented Jan 30, 2020

Unfortunately, I need to close this PR because it's in JavaScript and we have migrated to TypeScript since then.

I am going to continue our work in a different PR and I am going to ship it! Thank you for your work and apologies for it taking so long.

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.

6 participants