test(vue): add unit tests for getDisplayName utility#530
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis pull request adds unit tests for ChangesTest Coverage and Package Configuration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/vue/src/__tests__/utils/getDisplayName.test.ts (1)
26-59: ⚡ Quick winThe tests validate return order but not lookup keys.
Using only
mockReturnValueOncecan still pass ifgetDisplayNamecallsgetMappedUserProfileValuewith wrong attribute names but the same call count/order. Add argument assertions (e.g.,toHaveBeenNthCalledWith) or switch to a key-based mock implementation map.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/vue/src/__tests__/utils/getDisplayName.test.ts` around lines 26 - 59, The tests for getDisplayName currently only assert call order via mockReturnValueOnce, which can hide wrong lookup keys; update the tests in getDisplayName.test.ts to assert the actual lookup keys passed to getMappedUserProfileValue (or mockGet) — either replace the sequenced mockReturnValueOnce approach with a key-based mock implementation (e.g., mockGet.mockImplementation(key => ({ firstName: 'Jane', lastName: 'Doe', username: 'janedoe', email: 'jane@example.com' }[key]))) or add explicit argument assertions using toHaveBeenNthCalledWith against mockGet for each case; target the tests that call getDisplayName(mergedMappings, user, ...) and verify mockGet/getMappedUserProfileValue was called with the expected attribute names.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/vue/src/__tests__/utils/getDisplayName.test.ts`:
- Around line 33-47: Add a new unit test to cover the `name` fallback in
getDisplayName: in the test file using the same helpers (`mockGet`,
`mergedMappings`, `user`), set mockGet to return undefined for firstName,
lastName, username, and email (four consecutive mockReturnValueOnce(undefined)
calls) and then return a non-empty string for `name` (e.g., 'Jane Doe'); call
getDisplayName(mergedMappings, user) and assert the result equals that `name`
value so the explicit `name` branch in getDisplayName is exercised.
---
Nitpick comments:
In `@packages/vue/src/__tests__/utils/getDisplayName.test.ts`:
- Around line 26-59: The tests for getDisplayName currently only assert call
order via mockReturnValueOnce, which can hide wrong lookup keys; update the
tests in getDisplayName.test.ts to assert the actual lookup keys passed to
getMappedUserProfileValue (or mockGet) — either replace the sequenced
mockReturnValueOnce approach with a key-based mock implementation (e.g.,
mockGet.mockImplementation(key => ({ firstName: 'Jane', lastName: 'Doe',
username: 'janedoe', email: 'jane@example.com' }[key]))) or add explicit
argument assertions using toHaveBeenNthCalledWith against mockGet for each case;
target the tests that call getDisplayName(mergedMappings, user, ...) and verify
mockGet/getMappedUserProfileValue was called with the expected attribute names.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bf893e0e-aae8-48fc-a88f-6e6eb13031b1
📒 Files selected for processing (1)
packages/vue/src/__tests__/utils/getDisplayName.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@package.json`:
- Line 56: The CI workflows are using floating "latest" pnpm versions instead of
respecting the pinned version in package.json. Update all four CI workflow files
(e2e-tests.yml, npm-audit.yml, pr-builder.yml, and release.yml) to use the
pinned pnpm version 11.5.2 from package.json. Replace the matrix.pnpm-version:
[latest] configuration in e2e-tests, npm-audit, and pr-builder workflows with
the pinned version, and update the version field in the pnpm/action-setup
configuration in all workflows to use 11.5.2 instead of "latest" to ensure
consistency across all environments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dd5b73c2-60da-4815-8131-e3417ae366d7
📒 Files selected for processing (2)
package.jsonpackages/vue/src/__tests__/utils/getDisplayName.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/vue/src/tests/utils/getDisplayName.test.ts
| "access": "restricted" | ||
| } | ||
| }, | ||
| "packageManager": "pnpm@11.5.2+sha512.71c631e382066efc25625d5cf029075de07b61b37f6e27350fbd84b1bda5864c8c1967adc280776b45c30a715c0359a3be08fef42d5bb09e2b99029979692916" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import json, glob, re
with open("package.json", "r", encoding="utf-8") as f:
pkg = json.load(f)
pm = pkg.get("packageManager", "")
print("package.json packageManager:", pm)
pinned = None
if pm.startswith("pnpm@"):
pinned = pm.split("@", 1)[1].split("+", 1)[0]
print("Pinned pnpm version:", pinned)
for wf in sorted(glob.glob(".github/workflows/*.yml") + glob.glob(".github/workflows/*.yaml")):
text = open(wf, "r", encoding="utf-8").read()
if "pnpm/action-setup" not in text:
continue
m = re.search(r"pnpm-version:\s*\[([^\]]+)\]", text)
if m:
print(f"{wf}: matrix pnpm-version=[{m.group(1).strip()}]")
else:
print(f"{wf}: pnpm/action-setup found (no matrix pnpm-version pattern matched)")
PYRepository: asgardeo/javascript
Length of output: 645
🏁 Script executed:
#!/bin/bash
# Check release.yml for pnpm version handling
rg -A 5 "pnpm/action-setup" .github/workflows/release.ymlRepository: asgardeo/javascript
Length of output: 308
Align all CI workflows to use the pinned pnpm version from package.json.
Package.json pins pnpm@11.5.2, but all four CI workflows (e2e-tests.yml, npm-audit.yml, pr-builder.yml, release.yml) install from floating latest: e2e-tests, npm-audit, and pr-builder use matrix.pnpm-version: [latest], while release.yml uses version: latest in the pnpm/action-setup config. This allows pnpm to drift across environments, compromising reproducibility in testing and production builds. Update CI to respect the pinned version.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@package.json` at line 56, The CI workflows are using floating "latest" pnpm
versions instead of respecting the pinned version in package.json. Update all
four CI workflow files (e2e-tests.yml, npm-audit.yml, pr-builder.yml, and
release.yml) to use the pinned pnpm version 11.5.2 from package.json. Replace
the matrix.pnpm-version: [latest] configuration in e2e-tests, npm-audit, and
pr-builder workflows with the pinned version, and update the version field in
the pnpm/action-setup configuration in all workflows to use 11.5.2 instead of
"latest" to ensure consistency across all environments.
|
Added the missing test for the name fallback branch as suggested. Pushed in the latest commit. |
1 participant
Purpose
I noticed that the
getDisplayNameutility function in the@asgardeo/vuepackage had no unit tests at all. This PR adds 6 tests covering all the main code paths — including fallback behavior when user attributes are missing, and when displayAttributes are provided.Related Issues
@asgardeo/vueSDK #170Related PRs
Checklist
Security checks
Summary by CodeRabbit