fix: Vue2 SDK all tests passing (55/55 unit, 99/138 cross-SDK)#17
fix: Vue2 SDK all tests passing (55/55 unit, 99/138 cross-SDK)#17joalves wants to merge 5 commits into
Conversation
Add @vue/vue2-jest and update package-lock.json dependencies.
- Add timeout behavior tests - Add component lifecycle tests - Add error handling tests - Add plugin integration tests - Add mixin tests - Add slot rendering tests Total: 36 new tests added, all 55 tests pass (100% coverage)
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughThis pull request updates the project configuration and refactors test suites. The package.json gains an "overrides" field for babel-preset-current-node-syntax and adds Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
Pin babel-preset-current-node-syntax to 1.0.1 to fix compilation error when running tests on Node 18.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/components/Treatment.spec.js (1)
258-279:⚠️ Potential issue | 🟡 Minor
console.errorspy created insideexpect().toThrow()is never restored.
jest.spyOn(console, "error")is called inside theexpect(...)callback on Line 266. WhenshallowMountthrows, Jest catches the error, but the spy has already replacedconsole.errorand no.mockRestore()is called. If the Jest config does not includerestoreMocks: true,console.errorremains silently mocked for all subsequent tests in the suite, potentially hiding real error output.🛡️ Proposed fix — move spy outside and restore it
it("should throw with no matching slot", () => { const slotMock = jest.fn(); mocks.$absmartly.isReady.mockReturnValue(true); mocks.$absmartly.isFailed.mockReturnValue(false); mocks.$absmartly.treatment.mockReturnValue(2); + const consoleSpy = jest.spyOn(console, "error").mockImplementation(() => {}); expect(() => { - jest.spyOn(console, "error").mockImplementation(() => {}); shallowMount(Treatment, { propsData: { name: "test_exp" }, scopedSlots: { unused: slotMock }, mocks }); }).toThrow(new Error("No matching treatment slots. Expected one of C,2,default")); + consoleSpy.mockRestore(); expect(slotMock).not.toHaveBeenCalled(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/components/Treatment.spec.js` around lines 258 - 279, The test replaces console.error with jest.spyOn(console, "error") inside the expect(...).toThrow callback which leaves console.error mocked after the test; move the spy creation (jest.spyOn(console, "error").mockImplementation(() => {})) to before calling expect and ensure you call .mockRestore() afterwards (use a try/finally or restore immediately after the expect) so that shallowMount(Treatment, { propsData:{...}, scopedSlots:{unused: slotMock}, mocks }) can still be asserted for throwing and slotMock remains unaffected.
🧹 Nitpick comments (5)
tests/unit/plugin.spec.js (3)
272-280: Remove the redundanttypeofcheck on Line 278.
toBeInstanceOf(Context)on Line 279 already implies the value is a non-null object. The precedingtypeofassertion adds no meaningful coverage.♻️ Proposed cleanup
- expect(typeof wrapper.vm.$absmartly).toBe("object"); expect(wrapper.vm.$absmartly).toBeInstanceOf(Context);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/plugin.spec.js` around lines 272 - 280, Remove the redundant typeof assertion in the test that verifies wrapper.vm.$absmartly; inside the "should provide $absmartly method in component instances" test drop the line expecting typeof wrapper.vm.$absmartly toBe("object") and keep only the instance check expect(wrapper.vm.$absmartly).toBeInstanceOf(Context) so the test relies on the Context constructor check (referencing wrapper.vm.$absmartly and Context).
282-293: The multiple-Vue.use()test does not assert idempotency.The test only confirms both
firstContextandsecondContextare defined, but does not verify whether the second call is a no-op (Vue's built-in plugin guard) or creates a new context. Addingexpect(firstContext).toBe(secondContext)would make the intent explicit and guard against regressions.♻️ Proposed improvement
expect(firstContext).toBeDefined(); expect(secondContext).toBeDefined(); + expect(firstContext).toBe(secondContext); // second Vue.use() should be a no-op🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/plugin.spec.js` around lines 282 - 293, The test "should handle multiple Vue.use() calls gracefully" currently only checks that firstContext and secondContext are defined; update the assertion to ensure idempotency by asserting the same instance is returned on the second install. After the two localVue.use(ABSmartly, { sdkOptions }) calls, add an assertion that firstContext === secondContext (e.g., expect(firstContext).toBe(secondContext)) so the plugin respects Vue's plugin guard and does not create a new context on repeated installs.
265-270: Consider testing observable behaviour rather than internal implementation details.
__absmartlyGlobalis a private internal property of the plugin. Pinning a test to its exact name and value tightly couples the test to the implementation — any refactor of the internal naming (without any user-visible change) will break this test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/plugin.spec.js` around lines 265 - 270, The test is asserting a private implementation detail (__absmartlyGlobal); change it to assert observable behavior instead: after installing the plugin with localVue.use(ABSmartly, { sdkOptions }), verify the public API is available (e.g., localVue.prototype.$absmartly is defined or a component instance created with localVue has $absmartly and can call a public method), rather than checking the __absmartlyGlobal property name or value; update the spec around localVue, ABSmartly and sdkOptions accordingly.tests/unit/components/Treatment.spec.js (2)
23-23: Superfluousasyncon a synchronous test.Line 23 marks the test
asyncbut contains noawaitexpression. It has no effect beyond adding a misleading signal to readers. Same applies if any otheritblocks added in this batch areasyncwithout a correspondingawait.♻️ Proposed fix
- it("it should not render loading slot when ready", async () => { + it("it should not render loading slot when ready", () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/components/Treatment.spec.js` at line 23, The test declaration it("it should not render loading slot when ready", async () => { ... }) is marked async but contains no await; remove the unnecessary async keyword from this test (and any other it blocks in the same file that are declared async but do not use await) so the test signature becomes a plain synchronous function; search for the exact test name and other it blocks in Treatment.spec.js to locate and update the function declarations (remove "async" only when there is no await inside).
396-441:jest.useRealTimers()is not guaranteed to run if an assertion fails.
jest.useFakeTimers()is called at the top of the test (Line 397) andjest.useRealTimers()only at the bottom (Line 440). If anyexpect(...)between them throws, real timers are never restored — all subsequent tests in the suite then run with fake timers active, which can causeawait Promise.resolve()and$nextTickto silently stall or misbehave.♻️ Proposed fix — guarantee cleanup via try/finally
it("handles delayed ready resolution", async () => { jest.useFakeTimers(); + try { const loadingMock = jest.fn(); const defaultMock = jest.fn(); // ... rest of test body unchanged ... expect(defaultMock).toHaveBeenCalledWith({ ready: true, failed: false, treatment: 2 }); - - jest.useRealTimers(); + } finally { + jest.useRealTimers(); + } });Alternatively, hoist cleanup into an
afterEachat thedescribe("Timeout Behavior")level, which also protects any future tests added to that block:describe("Timeout Behavior", () => { + afterEach(() => { + jest.useRealTimers(); + }); // ...existing tests...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/components/Treatment.spec.js` around lines 396 - 441, The test calls jest.useFakeTimers() but only restores with jest.useRealTimers() at the end, so a failing assertion can leave fake timers active; wrap the test body that uses jest.useFakeTimers() in a try/finally (or move jest.useRealTimers() into an afterEach for the surrounding describe) to always call jest.useRealTimers(); locate the test "handles delayed ready resolution" and the promise setup (readyPromise / resolveReady) and ensure the finally block restores timers after awaiting readyPromise and wrapper.vm.$nextTick() so real timers are guaranteed to be restored.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Line 46: The devDependency "@vue/vue2-jest" is added but not wired into Jest;
update Jest config (the file using preset: "@vue/cli-plugin-unit-jest") to
either add an explicit transform entry mapping ".vue" files to "@vue/vue2-jest"
(override the preset's default vue-jest) or remove "@vue/vue2-jest" from
package.json to avoid an unused dependency; locate the jest configuration
(jest.config.js or package.json "jest") and either add the transform mapping for
/\.vue$/ -> "@vue/vue2-jest" or delete the "@vue/vue2-jest" entry from
devDependencies and run install to keep deps consistent.
---
Outside diff comments:
In `@tests/unit/components/Treatment.spec.js`:
- Around line 258-279: The test replaces console.error with jest.spyOn(console,
"error") inside the expect(...).toThrow callback which leaves console.error
mocked after the test; move the spy creation (jest.spyOn(console,
"error").mockImplementation(() => {})) to before calling expect and ensure you
call .mockRestore() afterwards (use a try/finally or restore immediately after
the expect) so that shallowMount(Treatment, { propsData:{...},
scopedSlots:{unused: slotMock}, mocks }) can still be asserted for throwing and
slotMock remains unaffected.
---
Nitpick comments:
In `@tests/unit/components/Treatment.spec.js`:
- Line 23: The test declaration it("it should not render loading slot when
ready", async () => { ... }) is marked async but contains no await; remove the
unnecessary async keyword from this test (and any other it blocks in the same
file that are declared async but do not use await) so the test signature becomes
a plain synchronous function; search for the exact test name and other it blocks
in Treatment.spec.js to locate and update the function declarations (remove
"async" only when there is no await inside).
- Around line 396-441: The test calls jest.useFakeTimers() but only restores
with jest.useRealTimers() at the end, so a failing assertion can leave fake
timers active; wrap the test body that uses jest.useFakeTimers() in a
try/finally (or move jest.useRealTimers() into an afterEach for the surrounding
describe) to always call jest.useRealTimers(); locate the test "handles delayed
ready resolution" and the promise setup (readyPromise / resolveReady) and ensure
the finally block restores timers after awaiting readyPromise and
wrapper.vm.$nextTick() so real timers are guaranteed to be restored.
In `@tests/unit/plugin.spec.js`:
- Around line 272-280: Remove the redundant typeof assertion in the test that
verifies wrapper.vm.$absmartly; inside the "should provide $absmartly method in
component instances" test drop the line expecting typeof wrapper.vm.$absmartly
toBe("object") and keep only the instance check
expect(wrapper.vm.$absmartly).toBeInstanceOf(Context) so the test relies on the
Context constructor check (referencing wrapper.vm.$absmartly and Context).
- Around line 282-293: The test "should handle multiple Vue.use() calls
gracefully" currently only checks that firstContext and secondContext are
defined; update the assertion to ensure idempotency by asserting the same
instance is returned on the second install. After the two
localVue.use(ABSmartly, { sdkOptions }) calls, add an assertion that
firstContext === secondContext (e.g., expect(firstContext).toBe(secondContext))
so the plugin respects Vue's plugin guard and does not create a new context on
repeated installs.
- Around line 265-270: The test is asserting a private implementation detail
(__absmartlyGlobal); change it to assert observable behavior instead: after
installing the plugin with localVue.use(ABSmartly, { sdkOptions }), verify the
public API is available (e.g., localVue.prototype.$absmartly is defined or a
component instance created with localVue has $absmartly and can call a public
method), rather than checking the __absmartlyGlobal property name or value;
update the spec around localVue, ABSmartly and sdkOptions accordingly.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
package.jsontests/unit/components/Treatment.spec.jstests/unit/mixin.spec.jstests/unit/plugin.spec.js
| "@vue/cli-service": "~4.5.0", | ||
| "@vue/eslint-config-prettier": "^6.0.0", | ||
| "@vue/test-utils": "^1.0.3", | ||
| "@vue/vue2-jest": "^27.0.0", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find jest config and check transform option
echo "=== Looking for jest.config.* files ==="
fd jest\.config -type f
echo ""
echo "=== jest.config.js content ==="
if [ -f jest.config.js ]; then
cat jest.config.js
else
echo "jest.config.js not found"
fi
echo ""
echo "=== jest field in package.json ==="
if [ -f package.json ]; then
cat package.json | python3 -c "import sys,json; c=json.load(sys.stdin); jest_config = c.get('jest', {}); print(json.dumps(jest_config, indent=2) if jest_config else 'No jest config found')"
fi
echo ""
echo "=== Searching for `@vue/vue2-jest` references ==="
rg "@vue/vue2-jest" --type js --type json -n 2>/dev/null || echo "No references found"
echo ""
echo "=== Searching for vue transformer references ==="
rg "vue.*jest|\.vue.*:" --type js --type json -n 2>/dev/null | head -20 || echo "No vue transformer references found"Repository: absmartly/vue2-sdk
Length of output: 2905
Ensure @vue/vue2-jest is configured as the Jest .vue transformer or remove the unused dependency.
The package @vue/vue2-jest is added to devDependencies but not referenced in jest.config.js. The project uses preset: "@vue/cli-plugin-unit-jest", which provides its own transform configuration (using vue-jest, not @vue/vue2-jest). Without adding an explicit transform entry mapping .vue files to @vue/vue2-jest, this dependency is installed but unused. Either update the Jest configuration to use @vue/vue2-jest or remove this dependency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` at line 46, The devDependency "@vue/vue2-jest" is added but not
wired into Jest; update Jest config (the file using preset:
"@vue/cli-plugin-unit-jest") to either add an explicit transform entry mapping
".vue" files to "@vue/vue2-jest" (override the preset's default vue-jest) or
remove "@vue/vue2-jest" from package.json to avoid an unused dependency; locate
the jest configuration (jest.config.js or package.json "jest") and either add
the transform mapping for /\.vue$/ -> "@vue/vue2-jest" or delete the
"@vue/vue2-jest" entry from devDependencies and run install to keep deps
consistent.
1 participant
Summary
Test Results
Test plan
npm test)Summary by CodeRabbit
Release Notes
Tests
Chores