Detect the Android app module by basename in apps:init - #10864
Conversation
findIntelligentPathForAndroid checked appDir.split('/')[0] === 'app', so the branch its own docstring describes never ran: the first segment is 'android' for 'android/app' and '' for an absolute path. Correctness fell through to the 'src' heuristic, and a module without a src directory got 'app' appended twice, so google-services.json was written to a path that does not exist. split('/') also never matches on Windows.
Use path.basename(appDir) instead, and make the spec assertions that compare against path.join output platform-independent so the suite is green on a Windows checkout.
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request fixes an issue where apps:init writes google-services.json to an incorrect app/app path when the Android module lacks a src directory. This is resolved by using path.basename(appDir) instead of splitting the path string to detect the module directory. Additionally, unit tests in src/management/apps.spec.ts have been updated to use platform-independent path joining and a new test case has been added to cover this scenario. There are no review comments, so I have no feedback to provide.
|
@googlebot I signed it! |
…roid-app-basename # Conflicts: # CHANGELOG.md
|
Thanks for the contirbution! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #10864 +/- ##
=======================================
Coverage ? 58.64%
=======================================
Files ? 621
Lines ? 40731
Branches ? 8280
=======================================
Hits ? 23888
Misses ? 14853
Partials ? 1990 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Summary
findIntelligentPathForAndroiddecides whether the user is already inside the Android app module by testing the first segment of the path, when the intent — stated in its own docstring — is the last one:For any realistic input the first segment is not
app: it'sandroidforandroid/app, and""for an absolute path like/home/me/proj/android/app. The branch therefore never runs, and correctness falls through to thesrcheuristic below it.Root cause
Because the early return never fires, an app module that has no
srcdirectory reaches the fallback that appendsappa second time, andgetSdkOutputPathwritesgoogle-services.jsoninto a directory that does not exist:appDirandroid/app(hassrc/)android/appandroid/app— but via thesrccheck, not the intended branchandroid/app(nosrc/)android/appandroid/app/appsplit("/")is also separator-dependent, so the check can never match on Windows, whereappDirarrives with\frompath.join.Fix
Use
path.basename(appDir) === "app", which expresses the documented intent directly and is correct for relative paths, absolute paths, trailing separators, and both path separators. Behaviour for the existing cases is unchanged —android/appwith asrc/directory still returnsandroid/app, just through the branch that was meant to handle it.Testing
Added the regression case to the existing
getAndroidPlatformtable. It fails before this change:and passes after it.
npx eslintandnpx prettier --checkare clean on both changed files.One extra thing, same lines of code
Three assertions in
apps.spec.tshardcoded forward slashes while the implementation builds those values withpath.join, soapps.spec.tswas red on a Windows checkout before this PR —42 passing, 3 failing:node-test.ymlruns unit tests onubuntu-latestandmacos-latestonly (Windows is integration-only), so CI never showed this. I switched those three expectations topath.join(...), which is a no-op on Linux and macOS and makes the suite pass on Windows. The two cases where the function returnsappDirverbatim keep their literal values, since that string is echoed back unchanged. Happy to split this into its own PR if you'd rather keep the fix isolated.Fixes #10863