Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMigrate the repository test stack from Jasmine to Mocha/Chai/Sinon: update docs and package configs, remove Jasmine typings and helpers, add a Mocha test helper, and convert many JS/TS spec files to use Chai assertions, Sinon spies/fake timers, and new prepare/restore test utilities. Changes
Sequence Diagram(s)sequenceDiagram
participant Karma as Karma (runner)
participant Frameworks as Frameworks (Mocha/Chai/Sinon)
participant Helper as mocha-test-helpers
participant Specs as Test Specs
participant Browser as Browser globals / DOM
Note over Karma,Frameworks: Test startup
Karma->>Frameworks: load `mocha`, `chai`, `sinon`
Karma->>Helper: include `mocha-test-helpers.js`
Helper->>Browser: install prepareTestNode, expect helpers, env flags (rgba(70,130,180,0.5))
Specs->>Helper: call prepareTestNode / restoreAfter in beforeEach
Specs->>Browser: exercise DOM, timers, spies
Specs->>Frameworks: report assertions (Chai/Sinon)
Frameworks->>Karma: test results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
Migrates the remaining Jasmine-era specs/config to a standardized Mocha + Chai + Sinon setup while keeping Karma browser execution during the transition.
Changes:
- Switched Karma framework configuration across packages/builds from Jasmine to Mocha/Chai/Sinon and removed Jasmine-related deps/docs.
- Converted many specs from Jasmine matchers/async helpers to Chai assertions, Sinon timers/spies, and async/await/Promise patterns.
- Added shared Mocha browser test helpers and updated build specs to use them.
Reviewed changes
Copilot reviewed 74 out of 149 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/binding.component/package.json | Switch Karma frameworks to mocha/chai/sinon for this package. |
| packages/bind/spec/nodePreprocessingBehaviors.ts | Convert Jasmine helpers/matchers to Mocha/Chai helpers. |
| packages/bind/spec/crossWindowBehaviors.ts | Replace Jasmine async patterns with try/finally cleanup and Chai assertions. |
| packages/bind/spec/bindingHandlerBehaviors.ts | Convert expectations to Chai and use Mocha test node helper. |
| packages/bind/spec/bindingCompletionPromiseBehavior.ts | Migrate promise specs to async/await and add cleanup restoration. |
| packages/bind/spec/asyncBindingBehaviors.ts | Replace Jasmine spies/clock with Sinon + helper utilities. |
| packages/bind/spec/arrayToDomEditDetectionBehaviors.ts | Convert matchers to Chai deep equality + shared helper assertions. |
| packages/bind/package.json | Switch Karma frameworks to mocha/chai/sinon for this package. |
| package.json | Remove Jasmine typings and karma-jasmine from devDependencies. |
| builds/knockout/spec/utilsDomBehaviors.js | Update expectations and test node setup for Mocha helpers. |
| builds/knockout/spec/taskBehaviors.js | Replace Jasmine clock/waits/runs with Sinon timers + async/await. |
| builds/knockout/spec/subscribableBehaviors.js | Convert matchers and feature flags to Mocha helper globals. |
| builds/knockout/spec/parseHtmlFragment.js | Use Mocha helper globals (prepareTestNode, ieVersion, expectContainHtml). |
| builds/knockout/spec/onErrorBehaviors.js | Replace waitsFor/runs with async + polling helper; set taskScheduler per test. |
| builds/knockout/spec/observableUtilsBehaviors.js | Convert Jasmine assertions to Chai assertions. |
| builds/knockout/spec/observableBehaviors.js | Convert matchers and throws assertions to Chai. |
| builds/knockout/spec/observableArrayChangeTrackingBehaviors.js | Convert array mutation assertions to Chai deep equality. |
| builds/knockout/spec/nodePreprocessingBehaviors.js | Update binding provider plumbing and assertions for Mocha helpers. |
| builds/knockout/spec/nativeTemplateEngineBehaviors.js | Replace jasmine matchers with helper assertions. |
| builds/knockout/spec/memoizationBehaviors.js | Convert expectations to Chai deep equality. |
| builds/knockout/spec/mappingHelperBehaviors.js | Convert throws/matchers to Chai assertions. |
| builds/knockout/spec/extenderBehaviors.js | Convert expectations to Chai deep equality. |
| builds/knockout/spec/expressionRewritingBehaviors.js | Convert expectations + translate xit to it.skip. |
| builds/knockout/spec/domNodeDisposalBehaviors.js | Replace Jasmine spies with Sinon stubs and Chai assertions. |
| builds/knockout/spec/dependentObservableDomBehaviors.js | Convert expectations to Chai deep equality and update test node helper. |
| builds/knockout/spec/defaultBindings/visibleHiddenBehaviors.js | Convert expectations and test node setup for Mocha helpers. |
| builds/knockout/spec/defaultBindings/usingBehaviors.js | Replace jasmine matchers with helper assertions and Chai expect. |
| builds/knockout/spec/defaultBindings/uniqueNameBehaviors.js | Convert expectations to Chai deep equality and test node helper. |
| builds/knockout/spec/defaultBindings/textBehaviors.js | Replace jasmine matchers with helper assertions and Chai expect. |
| builds/knockout/spec/defaultBindings/submitBehaviors.js | Convert expectations to Chai deep equality and test node helper. |
| builds/knockout/spec/defaultBindings/styleBehaviors.js | Replace toEqualOneOf with helper expectEqualOneOf; convert assertions. |
| builds/knockout/spec/defaultBindings/selectedOptionsBehaviors.js | Replace selection matchers with helper expectHaveSelectedValues. |
| builds/knockout/spec/defaultBindings/letBehaviors.js | Replace toContainText with helper expectContainText. |
| builds/knockout/spec/defaultBindings/ifnotBehaviors.js | Replace jasmine matchers with helper assertions and Chai expect. |
| builds/knockout/spec/defaultBindings/ifBehaviors.js | Replace jasmine matchers with helper assertions and convert xit to it.skip. |
| builds/knockout/spec/defaultBindings/htmlBehaviors.js | Replace toContainHtml with helper expectContainHtml. |
| builds/knockout/spec/defaultBindings/hasfocusBehaviors.js | Remove waits workaround and convert assertions to Chai. |
| builds/knockout/spec/defaultBindings/eventBehaviors.js | Convert expectations to Chai deep equality. |
| builds/knockout/spec/defaultBindings/enableDisableBehaviors.js | Convert enable/disable expectations to Chai deep equality. |
| builds/knockout/spec/defaultBindings/cssBehaviors.js | Convert class assertions to Chai deep equality and update test setup. |
| builds/knockout/spec/defaultBindings/clickBehaviors.js | Convert click binding expectations to Chai deep equality. |
| builds/knockout/spec/defaultBindings/attrBehaviors.js | Convert attr binding expectations to Chai + regex match. |
| builds/knockout/spec/crossWindowBehaviors.js | Replace waitsFor/runs with synchronous flow + try/finally cleanup. |
| builds/knockout/spec/components/loaderRegistryBehaviors.js | Replace waits/runs patterns with Promises and async assertions. |
| builds/knockout/spec/bindingPreprocessingBehaviors.js | Convert expectations and xit to it.skip. |
| builds/knockout/spec/bindingGlobalsBehaviors.js | Convert expectations and xit to it.skip. |
| builds/knockout/spec/asyncBindingBehaviors.js | Replace Jasmine clock/spies with Sinon fake timers/stubs. |
| builds/knockout/spec/arrayToDomEditDetectionBehaviors.js | Convert expectations and matchers to Chai/helper assertions. |
| builds/knockout/spec/arrayEditDetectionBehaviors.js | Convert compareArrays expectations to Chai deep equality. |
| builds/knockout/package.json | Switch Karma frameworks/files to mocha-test-helpers.js and mocha/chai/sinon. |
| builds/knockout/helpers/mocha-test-helpers.js | New shared Mocha browser helper utilities replacing Jasmine extensions. |
| builds/knockout/helpers/jasmine.extensions.js | Remove Jasmine-only browser helpers. |
| README.md | Update docs to reflect standardized Mocha/Chai/Sinon setup. |
| COVERAGE.md | Update coverage listing to reference mocha-test-helpers.ts vs jasmine helper. |
| AGENTS.md | Update contributor/testing guidance to Mocha/Chai/Sinon-only. |
Comment on lines
+8
to
+25
| function waitFor(condition, timeoutMs) { | ||
| timeoutMs = timeoutMs || 100; | ||
| return new Promise(function (resolve, reject) { | ||
| var deadline = Date.now() + timeoutMs; | ||
|
|
||
| function poll() { | ||
| if (condition()) { | ||
| resolve(); | ||
| return; | ||
| } | ||
|
|
||
| if (Date.now() >= deadline) { | ||
| reject(new Error('Timed out waiting for async error handling')); | ||
| return; | ||
| } | ||
|
|
||
| setTimeout(poll, 1); | ||
| } |
There was a problem hiding this comment.
waitFor defaults to a 100ms timeout, but these specs previously allowed substantially longer (waitsFor(..., 500) in the diff). This makes the async error tests more likely to flake under slower CI / Sauce environments. Consider increasing the default timeout (e.g., 500ms) or passing a higher timeout at call sites for these async error cases.
Member
There was a problem hiding this comment.
Probably also flaky with electron, but we can remove electron and sauce
|
|
||
| ko.applyBindings({ status: status, show: show }, testNode); | ||
| expect(testNode.childNodes[0]).toContainHtml(''); | ||
| expectContainText(testNode.childNodes[0], ''); |
There was a problem hiding this comment.
This assertion changed from an “HTML is empty” check to a “text is empty” check. expectContainText(..., '') can still pass even if the node contains non-text content (e.g., comment nodes or hidden DOM), which weakens the test and may allow regressions. Use expectContainHtml(testNode.childNodes[0], '') here to keep the original intent (“no DOM output yet”), consistent with the subsequent expectContainHtml assertions in the same test.
Suggested change
| expectContainText(testNode.childNodes[0], ''); | |
| expectContainHtml(testNode.childNodes[0], ''); |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a256781ad1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
packages/utils/src/tasks.ts
Outdated
| setTimeout(callback, 0) | ||
| } | ||
| } | ||
| options.taskScheduler = callback => queueMicrotask(callback) |
There was a problem hiding this comment.
options.taskScheduler is now hardwired to queueMicrotask, so in any runtime where that API is missing (for example older browsers/WebViews), the first call that schedules tasks will throw ReferenceError: queueMicrotask is not defined and deferred updates stop working. The previous implementation feature-detected scheduler support and fell back to setTimeout, so this commit introduces a runtime regression for non-queueMicrotask environments instead of degrading gracefully.
Useful? React with 👍 / 👎.
Member
There was a problem hiding this comment.
A fallback is already restored, it is only for IE11 necessary/relevant
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bind/spec/bindingAttributeBehaviors.ts (1)
46-57:⚠️ Potential issue | 🟠 MajorRegister the global option overrides for cleanup.
This setup overwrites
options.bindingProviderInstanceandoptions.onError, but neither is restored inafterEach. Because the provider is mutated throughout the suite, the last test can leak custom handlers and error behavior into later spec files.🧹 Minimal fix
beforeEach(function () { + restoreAfter(cleanups, options, 'bindingProviderInstance') + restoreAfter(cleanups, options as any, 'onError') + // Set up the default binding handlers. const provider = new MultiProvider({ providers: [new VirtualProvider(), new DataBindProvider()] })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bind/spec/bindingAttributeBehaviors.ts` around lines 46 - 57, The beforeEach setup replaces options.bindingProviderInstance and options.onError without restoring them, causing global state leakage; capture the original values (e.g., const originalProvider = options.bindingProviderInstance, const originalOnError = options.onError) when setting up provider and then add an afterEach that restores options.bindingProviderInstance = originalProvider and options.onError = originalOnError and clears any mutated provider.bindingHandlers (or reassigns a fresh provider) so each spec leaves global options unchanged; reference the beforeEach, provider, bindingHandlers, options.bindingProviderInstance and options.onError symbols to locate where to save and restore state.
🧹 Nitpick comments (5)
builds/knockout/spec/observableBehaviors.js (1)
5-36: Consider usingto.equalfor boolean assertions.For primitive boolean checks like
ko.isSubscribable(instance),to.equal(true)is more idiomatic thanto.deep.equal(true). While both work correctly,to.equalis the conventional choice for primitives and provides clearer intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builds/knockout/spec/observableBehaviors.js` around lines 5 - 36, Replace assertions that compare boolean primitives using to.deep.equal(...) with the more idiomatic to.equal(...). In the tests referencing ko.isSubscribable(instance), ko.isObservable(instance), ko.isComputed(instance), ko.isPureComputed(instance) and the array of values checked via ko.utils.arrayForEach, change expect(...).to.deep.equal(true/false) to expect(...).to.equal(true/false) so primitives use strict equality matcher.builds/knockout/spec/defaultBindings/ifnotBehaviors.js (1)
30-30: Useto.equalfor DOM node identity checks instead ofto.deep.equal.These assertions verify that the same DOM node reference is retained. Using
to.deep.equalperforms structural comparison rather than identity/reference equality. For checking object identity,to.equal(which uses strict===) is the correct matcher.Proposed fix
- expect(testNode.childNodes[0].childNodes[0]).to.deep.equal(originalNode); + expect(testNode.childNodes[0].childNodes[0]).to.equal(originalNode);Apply this change to both lines 30 and 35.
Also applies to: 35-35
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builds/knockout/spec/defaultBindings/ifnotBehaviors.js` at line 30, Replace structural equality assertions with identity checks: where the test uses expect(testNode.childNodes[0].childNodes[0]).to.deep.equal(originalNode) (and the similar assertion at the other occurrence), change the matcher to to.equal so the test asserts reference equality (strict ===) between the DOM nodes; update both occurrences that reference testNode and originalNode accordingly.builds/knockout/spec/asyncBindingBehaviors.js (1)
217-227: Node identity assertions should use strict equality.Lines 218 and 225 use
.to.deep.equal()for DOM node comparison. For object reference identity checks,.to.equal()(strict equality) is more appropriate:- expect(testNode.childNodes[0].childNodes[0]).to.deep.equal(originalNode); + expect(testNode.childNodes[0].childNodes[0]).to.equal(originalNode);Using
.to.equal()ensures the assertion fails if the nodes are different instances with the same structure, which is the intended behavior for verifying DOM node reuse.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builds/knockout/spec/asyncBindingBehaviors.js` around lines 217 - 227, Replace the DOM node identity assertions that use deep equality with strict equality: change the two expectations that call expect(testNode.childNodes[0].childNodes[0]).to.deep.equal(originalNode) to expect(...).to.equal(originalNode) so the tests verify object reference identity for testNode and originalNode (look for the two assertions around the comments about "previous SPAN remains" using testNode and originalNode).builds/knockout/spec/observableArrayBehaviors.js (1)
19-24: Consider using.to.equal()for primitive values.Using
.to.deep.equal()for boolean comparisons works but is unnecessarily verbose. For primitive values (booleans, numbers, strings),.to.equal()is more idiomatic:- expect(ko.isObservable(testObservableArray)).to.deep.equal(true); + expect(ko.isObservable(testObservableArray)).to.equal(true);Or even more idiomatic:
expect(ko.isObservable(testObservableArray)).to.be.true;This applies throughout the file for boolean/primitive assertions. Not blocking since
.to.deep.equal()is functionally correct.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builds/knockout/spec/observableArrayBehaviors.js` around lines 19 - 24, Replace uses of `.to.deep.equal(true)`/`.to.deep.equal(false)` in this spec with primitive-specific assertions for clarity and idiom: update assertions that call ko.isObservable, ko.isObservableArray, or any boolean-returning helpers (e.g., testObservableArray checks) to use `.to.equal(true)`/`.to.equal(false)` or the more idiomatic `.to.be.true`/`.to.be.false` instead of `.to.deep.equal(...)`; change each occurrence in observableArrayBehaviors.js accordingly (look for expect(ko.isObservable(...)) and expect(ko.isObservableArray(...)) calls).builds/knockout/spec/components/defaultLoaderBehaviors.js (1)
52-67: Prefer rejecting Promise-based async tests on assertion failures.At Line 52 and Line 62, the Promise wrappers only use
resolve. If an assertion throws inside the callback, rejection is not explicit. Addreject+try/catchfor deterministic async failure handling.Suggested patch
- return new Promise(function(resolve) { + return new Promise(function(resolve, reject) { ko.components.register(testComponentName, expectedConfig); ko.components.defaultLoader.getConfig(testComponentName, function(actualConfig) { - expect(actualConfig).to.equal(expectedConfig); - resolve(); + try { + expect(actualConfig).to.equal(expectedConfig); + resolve(); + } catch (error) { + reject(error); + } }); }); @@ - return new Promise(function(resolve) { + return new Promise(function(resolve, reject) { ko.components.defaultLoader.getConfig(testComponentName, function(actualConfig) { - expect(actualConfig).to.equal(null); - resolve(); + try { + expect(actualConfig).to.equal(null); + resolve(); + } catch (error) { + reject(error); + } }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builds/knockout/spec/components/defaultLoaderBehaviors.js` around lines 52 - 67, The Promise-based tests wrap async calls with only resolve, so assertion failures thrown inside the getConfig callback don't explicitly reject the Promise; update both test cases that call ko.components.defaultLoader.getConfig (the one registering testComponentName with expectedConfig and the one expecting null for unknown components) to accept both resolve and reject, and wrap the assertion code that checks actualConfig (expect(actualConfig).to.equal(...)) in a try/catch: on success call resolve(), on catch call reject(error) so the test harness gets deterministic rejections on assertion failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@builds/knockout/helpers/mocha-test-helpers.js`:
- Around line 4-8: The restoreAfter helper only saved the original value;
instead capture the original property descriptor and whether it was an own
property so teardown restores existence/descriptor, not just value: inside
restoreAfter (params cleanups, object, propertyName) get const
originalDescriptor = Object.getOwnPropertyDescriptor(object, propertyName) and
const hadOwn = Object.prototype.hasOwnProperty.call(object, propertyName), then
push a cleanup that if (originalDescriptor) uses Object.defineProperty(object,
propertyName, originalDescriptor) to restore descriptor, else if (hadOwn ===
false) deletes the own property (delete object[propertyName]) to reinstate
inherited/absent state; reference function restoreAfter, variables cleanups,
object, propertyName, and originalValue in your changes.
In `@builds/knockout/spec/bindingAttributeBehaviors.js`:
- Around line 27-28: Replace structural equality assertions with reference
equality for the view-model checks: locate the tests that assert the applied
view model (the expectations using viewModel and suppliedViewModel in the
applyBindings specs) and change expect(...).to.deep.equal(suppliedViewModel) to
expect(...).to.equal(suppliedViewModel) so the spec verifies the exact object is
forwarded by applyBindings.
In `@builds/knockout/spec/bindingGlobalsBehaviors.js`:
- Around line 13-14: Remove the redundant runtime skip inside the skipped test:
in the test using it.skip('is reassignable
(https://github.com/knockout/tko/issues/166)', function() { ... }), delete the
internal this.skip() call because it.skip already marks the test skipped and the
callback will not run; keep the it.skip call and its description but remove the
this.skip() line to avoid dead/unreachable code.
In `@builds/knockout/spec/components/componentBindingBehaviors.js`:
- Around line 104-105: The assertion compares the rendered node against
testTemplate[0] which is undefined because testTemplate is a DocumentFragment;
update the assertion to compare against the template's actual first child (e.g.
testTemplate.childNodes[0] or testTemplate.firstChild) so
expect(testNode.childNodes[0].childNodes[0]).not.to.equal(testTemplate.childNodes[0])
(locate this in the test using identifiers testNode and testTemplate).
In `@builds/knockout/spec/defaultBindings/valueBehaviors.js`:
- Around line 153-156: Test writes to the wrong input element: change the last
block that assigns to testNode.childNodes[0] to use the third input
(testNode.childNodes[2]) so the ['set'] round-trip actually exercises
getSetter()['set']; update both the .value assignment and the
ko.utils.triggerEvent call to target childNodes[2] and keep the
expect(mySetter.set).to.deep.equal('669').
In `@builds/knockout/spec/dependentObservableBehaviors.js`:
- Line 73: Replace the loose deep equality checks with strict identity checks
for the receiver assertions: change the assertions that use
expect(invokedWriteWithThis).to.deep.equal(function(){return this;}.call()),
expect(invokedWriteWithThis).to.deep.equal(someOwner),
expect(actualReadThis).to.deep.equal(expectedThis), and
expect(actualWriteThis).to.deep.equal(expectedThis) so they use to.equal()
instead of to.deep.equal(), ensuring the owner/this identity is asserted; update
the expectations referencing invokedWriteWithThis, someOwner, actualReadThis,
actualWriteThis, and expectedThis accordingly.
In `@builds/knockout/spec/templatingBehaviors.js`:
- Around line 59-66: The code currently replaces falsy caller-supplied values by
doing `data = data || {}` and then calls `ko.utils.extend(data,
options.templateRenderingVariablesInScope)`, which mutates the caller's model
and collapses valid falsy primitives; instead, preserve caller values and avoid
mutating them by creating a new local scope object (e.g., use the existing
`data` as-is even if 0/false/'' and only treat null/undefined as missing), then
merge `options.templateRenderingVariablesInScope` into a new object and use that
for template rendering; update the logic around `data`, `nomangle$data`, and the
`ko.utils.extend` call so the original `data` isn't mutated and falsy primitives
are preserved.
In `@packages/bind/spec/bindingAttributeBehaviors.ts`:
- Line 451: The one-line call to
objectForEach(contextFor(testNode.childNodes[0].childNodes[0]), prop =>
expect(allowedProperties).to.contain(prop)) is exceeding the repo's Prettier
width and failing CI; break this expression into multiple lines (or run
Prettier) so it fits the 120-char limit and matches repo style (no semicolons,
single quotes, trailing commas none), e.g. split the contextFor call and the
arrow callback across lines and/or assign contextFor(...) to a local (e.g.
nodeContext) before calling objectForEach to make the line shorter; update the
call sites using objectForEach, contextFor, testNode, childNodes,
allowedProperties, and expect accordingly.
In `@packages/bind/spec/bindingHandlerBehaviors.ts`:
- Around line 1-2: Prettier flagged formatting in this file; run the formatter
or reformat the import line for consistency (e.g., fix spacing/newlines around
the "import { expect } from 'chai'" statement). Execute `npx prettier --write
packages/bind/spec/bindingHandlerBehaviors.ts` or apply equivalent formatting so
the import of expect and any surrounding whitespace match your Prettier config
and CI will pass.
---
Outside diff comments:
In `@packages/bind/spec/bindingAttributeBehaviors.ts`:
- Around line 46-57: The beforeEach setup replaces
options.bindingProviderInstance and options.onError without restoring them,
causing global state leakage; capture the original values (e.g., const
originalProvider = options.bindingProviderInstance, const originalOnError =
options.onError) when setting up provider and then add an afterEach that
restores options.bindingProviderInstance = originalProvider and options.onError
= originalOnError and clears any mutated provider.bindingHandlers (or reassigns
a fresh provider) so each spec leaves global options unchanged; reference the
beforeEach, provider, bindingHandlers, options.bindingProviderInstance and
options.onError symbols to locate where to save and restore state.
---
Nitpick comments:
In `@builds/knockout/spec/asyncBindingBehaviors.js`:
- Around line 217-227: Replace the DOM node identity assertions that use deep
equality with strict equality: change the two expectations that call
expect(testNode.childNodes[0].childNodes[0]).to.deep.equal(originalNode) to
expect(...).to.equal(originalNode) so the tests verify object reference identity
for testNode and originalNode (look for the two assertions around the comments
about "previous SPAN remains" using testNode and originalNode).
In `@builds/knockout/spec/components/defaultLoaderBehaviors.js`:
- Around line 52-67: The Promise-based tests wrap async calls with only resolve,
so assertion failures thrown inside the getConfig callback don't explicitly
reject the Promise; update both test cases that call
ko.components.defaultLoader.getConfig (the one registering testComponentName
with expectedConfig and the one expecting null for unknown components) to accept
both resolve and reject, and wrap the assertion code that checks actualConfig
(expect(actualConfig).to.equal(...)) in a try/catch: on success call resolve(),
on catch call reject(error) so the test harness gets deterministic rejections on
assertion failures.
In `@builds/knockout/spec/defaultBindings/ifnotBehaviors.js`:
- Line 30: Replace structural equality assertions with identity checks: where
the test uses
expect(testNode.childNodes[0].childNodes[0]).to.deep.equal(originalNode) (and
the similar assertion at the other occurrence), change the matcher to to.equal
so the test asserts reference equality (strict ===) between the DOM nodes;
update both occurrences that reference testNode and originalNode accordingly.
In `@builds/knockout/spec/observableArrayBehaviors.js`:
- Around line 19-24: Replace uses of
`.to.deep.equal(true)`/`.to.deep.equal(false)` in this spec with
primitive-specific assertions for clarity and idiom: update assertions that call
ko.isObservable, ko.isObservableArray, or any boolean-returning helpers (e.g.,
testObservableArray checks) to use `.to.equal(true)`/`.to.equal(false)` or the
more idiomatic `.to.be.true`/`.to.be.false` instead of `.to.deep.equal(...)`;
change each occurrence in observableArrayBehaviors.js accordingly (look for
expect(ko.isObservable(...)) and expect(ko.isObservableArray(...)) calls).
In `@builds/knockout/spec/observableBehaviors.js`:
- Around line 5-36: Replace assertions that compare boolean primitives using
to.deep.equal(...) with the more idiomatic to.equal(...). In the tests
referencing ko.isSubscribable(instance), ko.isObservable(instance),
ko.isComputed(instance), ko.isPureComputed(instance) and the array of values
checked via ko.utils.arrayForEach, change expect(...).to.deep.equal(true/false)
to expect(...).to.equal(true/false) so primitives use strict equality matcher.
🪄 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: d7ac3690-b0ca-4ad9-8d44-5267e7933e05
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (148)
AGENTS.mdCOVERAGE.mdREADME.mdbuilds/knockout/helpers/jasmine.extensions.jsbuilds/knockout/helpers/mocha-test-helpers.jsbuilds/knockout/package.jsonbuilds/knockout/spec/arrayEditDetectionBehaviors.jsbuilds/knockout/spec/arrayToDomEditDetectionBehaviors.jsbuilds/knockout/spec/asyncBehaviors.jsbuilds/knockout/spec/asyncBindingBehaviors.jsbuilds/knockout/spec/bindingAttributeBehaviors.jsbuilds/knockout/spec/bindingDependencyBehaviors.jsbuilds/knockout/spec/bindingGlobalsBehaviors.jsbuilds/knockout/spec/bindingPreprocessingBehaviors.jsbuilds/knockout/spec/components/componentBindingBehaviors.jsbuilds/knockout/spec/components/customElementBehaviors.jsbuilds/knockout/spec/components/defaultLoaderBehaviors.jsbuilds/knockout/spec/components/loaderRegistryBehaviors.jsbuilds/knockout/spec/crossWindowBehaviors.jsbuilds/knockout/spec/defaultBindings/attrBehaviors.jsbuilds/knockout/spec/defaultBindings/checkedBehaviors.jsbuilds/knockout/spec/defaultBindings/clickBehaviors.jsbuilds/knockout/spec/defaultBindings/cssBehaviors.jsbuilds/knockout/spec/defaultBindings/enableDisableBehaviors.jsbuilds/knockout/spec/defaultBindings/eventBehaviors.jsbuilds/knockout/spec/defaultBindings/foreachBehaviors.jsbuilds/knockout/spec/defaultBindings/hasfocusBehaviors.jsbuilds/knockout/spec/defaultBindings/htmlBehaviors.jsbuilds/knockout/spec/defaultBindings/ifBehaviors.jsbuilds/knockout/spec/defaultBindings/ifnotBehaviors.jsbuilds/knockout/spec/defaultBindings/letBehaviors.jsbuilds/knockout/spec/defaultBindings/optionsBehaviors.jsbuilds/knockout/spec/defaultBindings/selectedOptionsBehaviors.jsbuilds/knockout/spec/defaultBindings/styleBehaviors.jsbuilds/knockout/spec/defaultBindings/submitBehaviors.jsbuilds/knockout/spec/defaultBindings/textBehaviors.jsbuilds/knockout/spec/defaultBindings/textInputBehaviors.jsbuilds/knockout/spec/defaultBindings/uniqueNameBehaviors.jsbuilds/knockout/spec/defaultBindings/usingBehaviors.jsbuilds/knockout/spec/defaultBindings/valueBehaviors.jsbuilds/knockout/spec/defaultBindings/visibleHiddenBehaviors.jsbuilds/knockout/spec/defaultBindings/withBehaviors.jsbuilds/knockout/spec/dependentObservableBehaviors.jsbuilds/knockout/spec/dependentObservableDomBehaviors.jsbuilds/knockout/spec/domNodeDisposalBehaviors.jsbuilds/knockout/spec/expressionRewritingBehaviors.jsbuilds/knockout/spec/extenderBehaviors.jsbuilds/knockout/spec/mappingHelperBehaviors.jsbuilds/knockout/spec/memoizationBehaviors.jsbuilds/knockout/spec/nativeTemplateEngineBehaviors.jsbuilds/knockout/spec/nodePreprocessingBehaviors.jsbuilds/knockout/spec/observableArrayBehaviors.jsbuilds/knockout/spec/observableArrayChangeTrackingBehaviors.jsbuilds/knockout/spec/observableBehaviors.jsbuilds/knockout/spec/observableUtilsBehaviors.jsbuilds/knockout/spec/onErrorBehaviors.jsbuilds/knockout/spec/parseHtmlFragment.jsbuilds/knockout/spec/pureComputedBehaviors.jsbuilds/knockout/spec/subscribableBehaviors.jsbuilds/knockout/spec/taskBehaviors.jsbuilds/knockout/spec/templatingBehaviors.jsbuilds/knockout/spec/utilsBehaviors.jsbuilds/knockout/spec/utilsDomBehaviors.jsglobal.d.tspackage.jsonpackages/bind/package.jsonpackages/bind/spec/arrayToDomEditDetectionBehaviors.tspackages/bind/spec/asyncBindingBehaviors.tspackages/bind/spec/bindingAttributeBehaviors.tspackages/bind/spec/bindingCompletionPromiseBehavior.tspackages/bind/spec/bindingDependencyBehaviors.tspackages/bind/spec/bindingHandlerBehaviors.tspackages/bind/spec/crossWindowBehaviors.tspackages/bind/spec/nodePreprocessingBehaviors.tspackages/binding.component/package.jsonpackages/binding.component/spec/componentBindingBehaviors.tspackages/binding.core/package.jsonpackages/binding.core/spec/attrBehaviors.tspackages/binding.core/spec/checkedBehaviors.tspackages/binding.core/spec/clickBehaviors.tspackages/binding.core/spec/cssBehaviors.tspackages/binding.core/spec/descendantsCompleteBehaviors.tspackages/binding.core/spec/enableDisableBehaviors.tspackages/binding.core/spec/eventBehaviors.tspackages/binding.core/spec/hasfocusBehaviors.tspackages/binding.core/spec/htmlBehaviors.tspackages/binding.core/spec/letBehaviors.tspackages/binding.core/spec/optionsBehaviors.tspackages/binding.core/spec/selectedOptionsBehaviors.tspackages/binding.core/spec/styleBehaviors.tspackages/binding.core/spec/submitBehaviors.tspackages/binding.core/spec/textBehaviors.tspackages/binding.core/spec/textInputBehaviors.tspackages/binding.core/spec/uniqueNameBehaviors.tspackages/binding.core/spec/usingBehaviors.tspackages/binding.core/spec/valueBehaviors.tspackages/binding.core/spec/visibleHiddenBehaviors.tspackages/binding.if/package.jsonpackages/binding.if/spec/elseBehaviors.tspackages/binding.if/spec/ifBehaviors.tspackages/binding.if/spec/ifnotBehaviors.tspackages/binding.if/spec/withBehaviors.tspackages/binding.template/package.jsonpackages/binding.template/spec/foreachBehaviors.tspackages/binding.template/spec/nativeTemplateEngineBehaviors.tspackages/binding.template/spec/templatingBehaviors.tspackages/computed/package.jsonpackages/computed/spec/asyncBehaviors.tspackages/computed/spec/computedDomBehaviors.tspackages/computed/spec/computedObservableBehaviors.tspackages/computed/spec/observableUtilsBehaviors.tspackages/computed/spec/proxyBehavior.tspackages/computed/spec/pureComputedBehaviors.tspackages/filter.punches/package.jsonpackages/filter.punches/spec/filterBehavior.tspackages/observable/package.jsonpackages/observable/spec/extenderBehaviors.tspackages/observable/spec/mappingHelperBehaviors.tspackages/observable/spec/observableArrayBehaviors.tspackages/observable/spec/observableArrayChangeTrackingBehaviors.tspackages/observable/spec/observableBehaviors.tspackages/observable/spec/subscribableBehaviors.tspackages/provider.component/package.jsonpackages/provider.component/spec/componentProviderBehaviors.tspackages/provider.component/spec/customElementBehaviors.tspackages/provider.mustache/package.jsonpackages/provider.mustache/spec/attributeInterpolationSpec.tspackages/provider.mustache/spec/textInterpolationSpec.tspackages/utils.component/package.jsonpackages/utils.component/spec/ComponentABCBehaviors.tspackages/utils.component/spec/defaultLoaderBehaviors.tspackages/utils.component/spec/loaderRegistryBehaviors.tspackages/utils.parser/spec/preparserBehavior.tspackages/utils/helpers/jasmine-13-helper.tspackages/utils/helpers/mocha-test-helpers.tspackages/utils/package.jsonpackages/utils/spec/arrayEditDetectionBehaviors.tspackages/utils/spec/domNodeDisposalBehaviors.tspackages/utils/spec/memoizationBehaviors.tspackages/utils/spec/onErrorBehaviors.tspackages/utils/spec/parseHtmlFragmentBehavior.tspackages/utils/spec/taskBehaviors.tspackages/utils/spec/utilsBehaviors.tspackages/utils/spec/utilsDomBehaviors.tspackages/utils/src/tasks.tsplans/testing-modernization.mdtko.io/package.jsontsconfig.json
💤 Files with no reviewable changes (3)
- package.json
- global.d.ts
- builds/knockout/helpers/jasmine.extensions.js
Comment on lines
+4
to
+8
| function restoreAfter(cleanups, object, propertyName) { | ||
| const originalValue = object[propertyName] | ||
| cleanups.push(function () { | ||
| object[propertyName] = originalValue | ||
| }) |
Contributor
There was a problem hiding this comment.
Restore property existence/descriptor, not just the value.
If the original member was inherited or absent, writing object[propertyName] = originalValue leaves behind an own property set to undefined. That changes later lookup behavior, which is risky for overrides like getComponentNameForNode.
Suggested fix
function restoreAfter(cleanups, object, propertyName) {
- const originalValue = object[propertyName]
+ const hadOwnProperty = Object.prototype.hasOwnProperty.call(object, propertyName)
+ const originalDescriptor = hadOwnProperty ? Object.getOwnPropertyDescriptor(object, propertyName) : undefined
cleanups.push(function () {
- object[propertyName] = originalValue
+ if (originalDescriptor) {
+ Object.defineProperty(object, propertyName, originalDescriptor)
+ } else {
+ delete object[propertyName]
+ }
})
}As per coding guidelines: builds/knockout/**: Preserve backwards compatibility in @tko/build.knockout.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@builds/knockout/helpers/mocha-test-helpers.js` around lines 4 - 8, The
restoreAfter helper only saved the original value; instead capture the original
property descriptor and whether it was an own property so teardown restores
existence/descriptor, not just value: inside restoreAfter (params cleanups,
object, propertyName) get const originalDescriptor =
Object.getOwnPropertyDescriptor(object, propertyName) and const hadOwn =
Object.prototype.hasOwnProperty.call(object, propertyName), then push a cleanup
that if (originalDescriptor) uses Object.defineProperty(object, propertyName,
originalDescriptor) to restore descriptor, else if (hadOwn === false) deletes
the own property (delete object[propertyName]) to reinstate inherited/absent
state; reference function restoreAfter, variables cleanups, object,
propertyName, and originalValue in your changes.
Comment on lines
+27
to
+28
| expect(element.id).to.deep.equal("testElement"); | ||
| expect(viewModel).to.deep.equal(suppliedViewModel); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n builds/knockout/spec/bindingAttributeBehaviors.js | head -60Repository: knockout/tko
Length of output: 3260
🏁 Script executed:
cd builds/knockout/spec && git log --oneline -20 bindingAttributeBehaviors.jsRepository: knockout/tko
Length of output: 97
🏁 Script executed:
rg "to\.(equal|deep\.equal)" builds/knockout/spec --type js -A 1 -B 1 | head -80Repository: knockout/tko
Length of output: 5328
🏁 Script executed:
# Verify the test intent by checking what suppliedViewModel is and how it's used
rg "suppliedViewModel" builds/knockout/spec/bindingAttributeBehaviors.js -B 2 -A 2Repository: knockout/tko
Length of output: 1291
Use equal for the view-model reference checks.
These specs are meant to verify that applyBindings forwards the exact model object. deep.equal will also accept a different object with the same shape, so using it here weakens the contract these tests are guarding. Since this is a backwards compatibility suite for @tko/build.knockout, the distinction between reference equality and structural equality is important.
Change lines 28 and 43 from to.deep.equal(suppliedViewModel) to to.equal(suppliedViewModel).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@builds/knockout/spec/bindingAttributeBehaviors.js` around lines 27 - 28,
Replace structural equality assertions with reference equality for the
view-model checks: locate the tests that assert the applied view model (the
expectations using viewModel and suppliedViewModel in the applyBindings specs)
and change expect(...).to.deep.equal(suppliedViewModel) to
expect(...).to.equal(suppliedViewModel) so the spec verifies the exact object is
forwarded by applyBindings.
Comment on lines
+13
to
14
| it.skip('is reassignable (https://github.com/knockout/tko/issues/166)', function() { | ||
| this.skip(); |
Contributor
There was a problem hiding this comment.
Redundant this.skip() call inside it.skip.
When using it.skip(...), the test is already marked as skipped at the framework level. Calling this.skip() inside the test body is unnecessary and will never execute since the test callback is not invoked for skipped tests. Remove line 14.
Proposed fix
it.skip('is reassignable (https://github.com/knockout/tko/issues/166)', function() {
- this.skip();
ko.options.bindingGlobals = {foo: "bar"};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it.skip('is reassignable (https://github.com/knockout/tko/issues/166)', function() { | |
| this.skip(); | |
| it.skip('is reassignable (https://github.com/knockout/tko/issues/166)', function() { | |
| ko.options.bindingGlobals = {foo: "bar"}; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@builds/knockout/spec/bindingGlobalsBehaviors.js` around lines 13 - 14, Remove
the redundant runtime skip inside the skipped test: in the test using
it.skip('is reassignable (https://github.com/knockout/tko/issues/166)',
function() { ... }), delete the internal this.skip() call because it.skip
already marks the test skipped and the callback will not run; keep the it.skip
call and its description but remove the this.skip() line to avoid
dead/unreachable code.
Comment on lines
104
to
+105
| // Also be sure it's a clone | ||
| expect(testNode.childNodes[0].childNodes[0]).not.toBe(testTemplate[0]); | ||
| expect(testNode.childNodes[0].childNodes[0]).not.to.equal(testTemplate[0]); |
Contributor
There was a problem hiding this comment.
This clone assertion is currently comparing against undefined.
testTemplate is a DocumentFragment, so testTemplate[0] is always undefined. The assertion no longer verifies that the rendered node is a clone.
Suggested fix
- expect(testNode.childNodes[0].childNodes[0]).not.to.equal(testTemplate[0]);
+ expect(testNode.childNodes[0].childNodes[0]).not.to.equal(testTemplate.childNodes[0]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@builds/knockout/spec/components/componentBindingBehaviors.js` around lines
104 - 105, The assertion compares the rendered node against testTemplate[0]
which is undefined because testTemplate is a DocumentFragment; update the
assertion to compare against the template's actual first child (e.g.
testTemplate.childNodes[0] or testTemplate.firstChild) so
expect(testNode.childNodes[0].childNodes[0]).not.to.equal(testTemplate.childNodes[0])
(locate this in the test using identifiers testNode and testTemplate).
Member
There was a problem hiding this comment.
1:1 porting of the existing logic.
Contributor
There was a problem hiding this comment.
@phillipc, understood — this is a pre-existing issue that predates this PR and is outside the scope of the migration. Would you like me to open a follow-up GitHub issue to track fixing this assertion so testTemplate.childNodes[0] is used instead of testTemplate[0]?
🧠 Learnings used
Learnt from: CR
Repo: knockout/tko PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-05T17:45:34.212Z
Learning: Applies to packages/*/spec/**/*.{ts,tsx,js,jsx} : Write new tests using Mocha, Chai, and Sinon frameworks (not legacy Jasmine 1.3)
Learnt from: CR
Repo: knockout/tko PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-05T17:45:34.212Z
Learning: Applies to builds/knockout/**/*.{ts,tsx,js,jsx} : Preserve backwards compatibility in tko/build.knockout
Comment on lines
+77
to
+87
| var scope = Object.assign({ | ||
| bindingContext: bindingContext, | ||
| ko: ko, | ||
| nomangle$data: nomangle$data, | ||
| root: data, | ||
| templateOptions: templateOptions, | ||
| unwrap: ko.utils.unwrapObservable | ||
| }, bindingContext, data, options.templateRenderingVariablesInScope); | ||
| with (scope) { | ||
| evalResult = eval(script); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and read the relevant section
cat -n builds/knockout/spec/templatingBehaviors.js | head -100 | tail -40Repository: knockout/tko
Length of output: 2368
🏁 Script executed:
# Check for export statements in the file
rg -n "^export|^import" builds/knockout/spec/templatingBehaviors.js | head -20Repository: knockout/tko
Length of output: 113
🏁 Script executed:
# Get file size to understand full context
wc -l builds/knockout/spec/templatingBehaviors.jsRepository: knockout/tko
Length of output: 104
🏁 Script executed:
# Check the beginning of the file for any special configuration or comments
head -20 builds/knockout/spec/templatingBehaviors.jsRepository: knockout/tko
Length of output: 611
🏁 Script executed:
# Look for build configuration files that might transpile or bundle this
find . -maxdepth 2 -name "webpack.config.*" -o -name "tsconfig.json" -o -name ".babelrc*" -o -name "jest.config.*" | head -10Repository: knockout/tko
Length of output: 71
🏁 Script executed:
# Check if there's a package.json and build scripts
cat package.json | grep -A 20 '"scripts"' | head -25Repository: knockout/tko
Length of output: 593
🏁 Script executed:
# Check tsconfig.json to see if there's transpilation that might affect this
cat tsconfig.jsonRepository: knockout/tko
Length of output: 1092
🏁 Script executed:
# Check if this file is actually part of the build or if it's a test-only artifact
find . -name "*.json" -path "*/builds/knockout/*" | head -10Repository: knockout/tko
Length of output: 86
🏁 Script executed:
# Look for any configuration that might process this spec file
find . -name "karma.conf.*" -o -name "jest.config.*" -o -name "mocha.opts" | head -5Repository: knockout/tko
Length of output: 77
🏁 Script executed:
# Check the builds/knockout/package.json to see if it has specific build configuration
cat builds/knockout/package.jsonRepository: knockout/tko
Length of output: 2039
🏁 Script executed:
# Check karma configuration to see how tests are run
cat tools/karma.conf.js | head -100Repository: knockout/tko
Length of output: 2642
🏁 Script executed:
# Look for any build scripts or webpack configuration in builds/knockout
find builds/knockout -name "*.config.*" -o -name "build.*" -o -name "*.mk" | head -10Repository: knockout/tko
Length of output: 38
🏁 Script executed:
# Check if the code actually parses as valid JavaScript or if it's a syntax error
# Use Node.js to parse the file
node <<'EOF'
const fs = require('fs');
const path = require('path');
try {
const content = fs.readFileSync('builds/knockout/spec/templatingBehaviors.js', 'utf8');
new Function(content);
console.log("✓ File parses successfully as JavaScript");
} catch (e) {
console.log("✗ SYNTAX ERROR:", e.message);
console.log("At:", e.stack.split('\n')[1]);
}
EOFRepository: knockout/tko
Length of output: 135
🏁 Script executed:
# Check if esbuild is actually available and what format it uses
cat tools/karma.conf.js | grep -A 10 "esbuild:"Repository: knockout/tko
Length of output: 287
🏁 Script executed:
# Search for any 'use strict' directives in the file
rg "use strict" builds/knockout/spec/templatingBehaviors.jsRepository: knockout/tko
Length of output: 38
🏁 Script executed:
# Check if esbuild actually converts this successfully and what the output looks like
# Try to see if esbuild can process this file
npm list esbuild 2>/dev/null | head -5Repository: knockout/tko
Length of output: 92
🏁 Script executed:
# Look for test logs or CI configuration to see if tests are actually failing
find . -name ".github" -o -name "*.yml" -o -name "*.yaml" | grep -E "(workflow|ci)" | head -5Repository: knockout/tko
Length of output: 243
🏁 Script executed:
# Check if the spec file is actually being run/executed
rg -n "templatingBehaviors" . --type json --type yaml --type yml 2>/dev/null | head -10Repository: knockout/tko
Length of output: 38
🏁 Script executed:
# Check the main build workflow to understand how tests are executed
cat .github/workflows/main-build.ymlRepository: knockout/tko
Length of output: 991
🏁 Script executed:
# Check the electron workflow since it's also testing
cat .github/workflows/electron.yaml | head -60Repository: knockout/tko
Length of output: 624
🏁 Script executed:
# Check the Makefile to understand test-headless
cat Makefile | head -100Repository: knockout/tko
Length of output: 2338
🏁 Script executed:
# Search for any esbuild configuration that might convert the with statement
rg "with" builds/knockout/spec/templatingBehaviors.js -B 2 -A 2Repository: knockout/tko
Length of output: 9131
🏁 Script executed:
# Check if there's any build output directory that might show transpiled code
ls -la builds/knockout/dist/ 2>/dev/null | head -20Repository: knockout/tko
Length of output: 38
🏁 Script executed:
# Final check: See if there are any recent issue reports or comments about this specific problem
rg -i "with.*scope|strict.*mode" builds/knockout/spec/ --type jsRepository: knockout/tko
Length of output: 130
🏁 Script executed:
# Check if there are any eslint configs that might be suppressing this error
cat .eslintrc* 2>/dev/null || cat package.json | grep -A 20 '"eslintConfig"'Repository: knockout/tko
Length of output: 38
🏁 Script executed:
# Verify if the build is actually passing despite this syntax error
git log --oneline -10 2>/dev/null | head -5Repository: knockout/tko
Length of output: 97
Fix syntax error: with statement is forbidden in strict mode.
This file uses export (line 2), making it an ES module with implicit strict mode. The with (scope) block at lines 85–87 is a syntax error in strict mode and prevents the entire spec file from being parsed.
Move the scope injection into a non-module evaluator or use explicit variable bindings instead.
🧰 Tools
🪛 Biome (2.4.9)
[error] 85-87: with statements are not allowed in strict mode
(parse)
[error] 86-86: eval() exposes to security risks and performance issues.
(lint/security/noGlobalEval)
| allowedProperties.push('_subscribable') | ||
| } | ||
| objectForEach(contextFor(testNode.childNodes[0].childNodes[0]), prop => expect(allowedProperties).toContain(prop)) | ||
| objectForEach(contextFor(testNode.childNodes[0].childNodes[0]), prop => expect(allowedProperties).to.contain(prop)) |
Contributor
There was a problem hiding this comment.
This line is still tripping the Prettier check.
CI already reports a formatting failure for this file, and this one-liner is over the repo's configured width. Run Prettier or wrap it here.
🎨 Minimal wrap
- objectForEach(contextFor(testNode.childNodes[0].childNodes[0]), prop => expect(allowedProperties).to.contain(prop))
+ objectForEach(contextFor(testNode.childNodes[0].childNodes[0]), prop =>
+ expect(allowedProperties).to.contain(prop)
+ )As per coding guidelines, "Use Prettier for code formatting with no semicolons, single quotes, trailing commas: none, 120 char width."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| objectForEach(contextFor(testNode.childNodes[0].childNodes[0]), prop => expect(allowedProperties).to.contain(prop)) | |
| objectForEach(contextFor(testNode.childNodes[0].childNodes[0]), prop => | |
| expect(allowedProperties).to.contain(prop) | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bind/spec/bindingAttributeBehaviors.ts` at line 451, The one-line
call to objectForEach(contextFor(testNode.childNodes[0].childNodes[0]), prop =>
expect(allowedProperties).to.contain(prop)) is exceeding the repo's Prettier
width and failing CI; break this expression into multiple lines (or run
Prettier) so it fits the 120-char limit and matches repo style (no semicolons,
single quotes, trailing commas none), e.g. split the contextFor call and the
arrow callback across lines and/or assign contextFor(...) to a local (e.g.
nodeContext) before calling objectForEach to make the line shorter; update the
call sites using objectForEach, contextFor, testNode, childNodes,
allowedProperties, and expect accordingly.
Member
There was a problem hiding this comment.
@brianmhunt Try running "prettier" once, then the linting will continue. I would definitely separate these checks again.
Contributor
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/bind/spec/bindingAttributeBehaviors.ts (1)
451-451:⚠️ Potential issue | 🟡 MinorLine 451 still needs wrapping to satisfy Prettier width.
This one-liner appears to be the remaining formatting blocker in CI.
🎨 Minimal wrap
- objectForEach(contextFor(testNode.childNodes[0].childNodes[0]), prop => expect(allowedProperties).to.contain(prop)) + objectForEach(contextFor(testNode.childNodes[0].childNodes[0]), prop => + expect(allowedProperties).to.contain(prop) + )As per coding guidelines, "Use Prettier for code formatting with no semicolons, single quotes, trailing commas: none, 120 char width."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bind/spec/bindingAttributeBehaviors.ts` at line 451, The one-line call to objectForEach at line with objectForEach(contextFor(testNode.childNodes[0].childNodes[0]), prop => expect(allowedProperties).to.contain(prop)) is exceeding Prettier width; refactor it by breaking the expression across multiple lines (for example extract contextFor(...) or the childNodes access into a short variable like node = contextFor(testNode.childNodes[0].childNodes[0]) or place the callback on its own line) so that objectForEach(..., prop => expect(...)) is wrapped to satisfy the 120-char Prettier rule while keeping the same calls to objectForEach, contextFor, testNode, allowedProperties and expect.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/bind/spec/bindingAttributeBehaviors.ts`:
- Line 451: The one-line call to objectForEach at line with
objectForEach(contextFor(testNode.childNodes[0].childNodes[0]), prop =>
expect(allowedProperties).to.contain(prop)) is exceeding Prettier width;
refactor it by breaking the expression across multiple lines (for example
extract contextFor(...) or the childNodes access into a short variable like node
= contextFor(testNode.childNodes[0].childNodes[0]) or place the callback on its
own line) so that objectForEach(..., prop => expect(...)) is wrapped to satisfy
the 120-char Prettier rule while keeping the same calls to objectForEach,
contextFor, testNode, allowedProperties and expect.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 29464c6f-3e90-4f1f-a161-5c756fa8591e
📒 Files selected for processing (5)
builds/knockout/spec/bindingAttributeBehaviors.jspackages/bind/spec/bindingAttributeBehaviors.tspackages/binding.core/spec/valueBehaviors.tspackages/binding.foreach/spec/eachBehavior.tspackages/utils/spec/onErrorBehaviors.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- builds/knockout/spec/bindingAttributeBehaviors.js
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bind/spec/asyncBindingBehaviors.ts`:
- Around line 64-69: The test currently calls
expect(tasks.resetForTesting()).to.equal(0) before running teardown code
(cleanups.pop(), clock.restore(), resetting bindingHandlers.test and
bindingSpy), so a failing assertion can skip cleanup and leak state; update the
test to always run the cleanup regardless of the assertion by moving the cleanup
steps to a finally block or run them before the expect, ensuring cleanups.pop()
loop, clock.restore(), and resetting bindingHandlers.test / bindingSpy always
execute even if tasks.resetForTesting() throws or the expectation fails.
🪄 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: 1fd84f60-bed5-4f07-90b7-3c690cab2954
📒 Files selected for processing (9)
packages/bind/spec/asyncBindingBehaviors.tspackages/binding.if/spec/ifBehaviors.tspackages/binding.if/spec/ifnotBehaviors.tspackages/computed/spec/asyncBehaviors.tspackages/computed/spec/computedObservableBehaviors.tspackages/computed/spec/pureComputedBehaviors.tspackages/provider.component/spec/componentProviderBehaviors.tspackages/utils/spec/utilsBehaviors.tspackages/utils/src/tasks.ts
Comment on lines
64
to
69
| expect(tasks.resetForTesting()).to.equal(0) | ||
| while (cleanups.length) { | ||
| cleanups.pop()!() | ||
| } | ||
| clock.restore() | ||
| bindingSpy = bindingHandlers.test = null |
Contributor
There was a problem hiding this comment.
Make teardown cleanup unconditional before asserting pending tasks.
At Line 64, a failing expect(tasks.resetForTesting()).to.equal(0) can prevent cleanup execution (Lines 65-69), leaking fake timers/global options into later tests.
Proposed fix
afterEach(function () {
- expect(tasks.resetForTesting()).to.equal(0)
- while (cleanups.length) {
- cleanups.pop()!()
- }
- clock.restore()
- bindingSpy = bindingHandlers.test = null
+ const pendingTasks = tasks.resetForTesting()
+ try {
+ while (cleanups.length) {
+ cleanups.pop()!()
+ }
+ } finally {
+ clock.restore()
+ bindingSpy = bindingHandlers.test = null
+ }
+ expect(pendingTasks).to.equal(0)
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| expect(tasks.resetForTesting()).to.equal(0) | |
| while (cleanups.length) { | |
| cleanups.pop()!() | |
| } | |
| clock.restore() | |
| bindingSpy = bindingHandlers.test = null | |
| afterEach(function () { | |
| const pendingTasks = tasks.resetForTesting() | |
| try { | |
| while (cleanups.length) { | |
| cleanups.pop()!() | |
| } | |
| } finally { | |
| clock.restore() | |
| bindingSpy = bindingHandlers.test = null | |
| } | |
| expect(pendingTasks).to.equal(0) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bind/spec/asyncBindingBehaviors.ts` around lines 64 - 69, The test
currently calls expect(tasks.resetForTesting()).to.equal(0) before running
teardown code (cleanups.pop(), clock.restore(), resetting bindingHandlers.test
and bindingSpy), so a failing assertion can skip cleanup and leak state; update
the test to always run the cleanup regardless of the assertion by moving the
cleanup steps to a finally block or run them before the expect, ensuring
cleanups.pop() loop, clock.restore(), and resetting bindingHandlers.test /
bindingSpy always execute even if tasks.resetForTesting() throws or the
expectation fails.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
builds/knockout/spec/components/loaderRegistryBehaviors.js (1)
61-78: HardenwaitForerror handling and timeout defaulting.
condition()exceptions currently escape from the polling callback, and timeout defaulting uses a falsy check. Rejecting on condition errors and using explicit numeric defaulting makes failures cleaner and less brittle.♻️ Proposed patch
- waitFor = function(condition, timeoutMs) { - timeoutMs = timeoutMs || 1000; + waitFor = function(condition, timeoutMs) { + timeoutMs = typeof timeoutMs === 'number' ? timeoutMs : 1000; return new Promise(function(resolve, reject) { var deadline = Date.now() + timeoutMs; function poll() { - if (condition()) { + var ready; + try { + ready = condition(); + } catch (error) { + reject(error); + return; + } + + if (ready) { resolve(); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builds/knockout/spec/components/loaderRegistryBehaviors.js` around lines 61 - 78, The waitFor function should explicitly default timeoutMs when it's undefined and catch exceptions thrown by condition(): change the defaulting to something like "timeoutMs = (typeof timeoutMs === 'number') ? timeoutMs : 1000" and wrap the condition() call inside a try/catch within poll so any thrown error causes reject(error) instead of bubbling; keep the existing deadline logic and ensure reject is only called once when the deadline is exceeded or on condition errors while resolve is called when condition() returns truthy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@builds/knockout/spec/components/loaderRegistryBehaviors.js`:
- Around line 61-78: The waitFor function should explicitly default timeoutMs
when it's undefined and catch exceptions thrown by condition(): change the
defaulting to something like "timeoutMs = (typeof timeoutMs === 'number') ?
timeoutMs : 1000" and wrap the condition() call inside a try/catch within poll
so any thrown error causes reject(error) instead of bubbling; keep the existing
deadline logic and ensure reject is only called once when the deadline is
exceeded or on condition errors while resolve is called when condition() returns
truthy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5fa1d5b1-a8a1-4eb8-962b-496a22672065
📒 Files selected for processing (2)
builds/knockout/spec/asyncBindingBehaviors.jsbuilds/knockout/spec/components/loaderRegistryBehaviors.js
🚧 Files skipped from review as they are similar to previous changes (1)
- builds/knockout/spec/asyncBindingBehaviors.js
| templateOptions: templateOptions, | ||
| unwrap: ko.utils.unwrapObservable | ||
| }, bindingContext, scopeData); | ||
| with (scope) { |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
builds/knockout/spec/templatingBehaviors.js (1)
80-90:⚠️ Potential issue | 🔴 CriticalBlocker:
within module strict mode can make this file unparsable.This file has
exportat Line 2 (module context) and useswith (scope)here. In strict mode,withis invalid syntax, so this can fail before tests even execute.🐛 Suggested fix (remove
with, preserve scoped expression evaluation)- var evalResult; - var scope = Object.assign({ + var evalResult; + var scope = Object.assign({ bindingContext: bindingContext, ko: ko, nomangle$data: nomangle$data, root: renderData, templateOptions: templateOptions, unwrap: ko.utils.unwrapObservable }, bindingContext, scopeData); - with (scope) { - evalResult = eval(script); - } + var keys = Object.keys(scope); + var values = keys.map(function (k) { return scope[k]; }); + var fn = Function.apply(null, keys.concat( + '"use strict"; return (function(){ return (' + script + '); })();' + )); + evalResult = fn.apply(null, values);#!/bin/bash # Verify strict-mode hazard: module export + with statement in same file. # Expected: both lines are present; if yes, this is a language-level parse risk. rg -n '^\s*export\b|^\s*with\s*\(' builds/knockout/spec/templatingBehaviors.js🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builds/knockout/spec/templatingBehaviors.js` around lines 80 - 90, The use of "with (scope)" makes this module unparseable in strict/module mode; replace the with-based evaluation by creating a new function that receives the scope properties as parameters and invokes eval/script inside that function so properties (bindingContext, ko, nomangle$data, root/renderData, templateOptions, unwrap) are available as local variables; specifically modify the block that builds "scope" and the "with (scope) { evalResult = eval(script); }" to instead call a function (e.g., a generated evaluator) that accepts the unique symbols bindingContext, ko, nomangle$data, renderData (root), templateOptions, and ko.utils.unwrapObservable (unwrap) and returns the evaluated result, then assign that return value to evalResult.
🧹 Nitpick comments (1)
builds/knockout/spec/templatingBehaviors.js (1)
482-488: Align suite cleanup with migrated binding provider API.These tests now mutate
ko.options.bindingProviderInstance, but suite teardown still resetsko.bindingProvider.instance. Consider resetting both inafterEachto prevent cross-test leakage if a test missesrestoreAfter.♻️ Small hardening patch
afterEach(function() { ko.setTemplateEngine(new ko.nativeTemplateEngine()); - ko.bindingProvider.instance = undefined + ko.bindingProvider.instance = undefined + ko.options.bindingProviderInstance = undefined });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builds/knockout/spec/templatingBehaviors.js` around lines 482 - 488, The teardown currently only restores ko.bindingProvider.instance but tests now mutate ko.options.bindingProviderInstance too; update the suite cleanup (afterEach/restore logic) to reset both ko.options.bindingProviderInstance and ko.bindingProvider.instance (or call restoreAfter for ko.options.bindingProviderInstance where applicable) so tests cannot leak modified binding providers; reference the mutated symbols ko.options.bindingProviderInstance and ko.bindingProvider.instance and ensure both are restored to their originals in the afterEach/restore path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@builds/knockout/spec/templatingBehaviors.js`:
- Around line 80-90: The use of "with (scope)" makes this module unparseable in
strict/module mode; replace the with-based evaluation by creating a new function
that receives the scope properties as parameters and invokes eval/script inside
that function so properties (bindingContext, ko, nomangle$data, root/renderData,
templateOptions, unwrap) are available as local variables; specifically modify
the block that builds "scope" and the "with (scope) { evalResult = eval(script);
}" to instead call a function (e.g., a generated evaluator) that accepts the
unique symbols bindingContext, ko, nomangle$data, renderData (root),
templateOptions, and ko.utils.unwrapObservable (unwrap) and returns the
evaluated result, then assign that return value to evalResult.
---
Nitpick comments:
In `@builds/knockout/spec/templatingBehaviors.js`:
- Around line 482-488: The teardown currently only restores
ko.bindingProvider.instance but tests now mutate
ko.options.bindingProviderInstance too; update the suite cleanup
(afterEach/restore logic) to reset both ko.options.bindingProviderInstance and
ko.bindingProvider.instance (or call restoreAfter for
ko.options.bindingProviderInstance where applicable) so tests cannot leak
modified binding providers; reference the mutated symbols
ko.options.bindingProviderInstance and ko.bindingProvider.instance and ensure
both are restored to their originals in the afterEach/restore path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 11406a3a-b54a-4d86-9d93-0f9058a7bece
📒 Files selected for processing (6)
builds/knockout/spec/defaultBindings/valueBehaviors.jsbuilds/knockout/spec/dependentObservableBehaviors.jsbuilds/knockout/spec/templatingBehaviors.jspackages/bind/spec/asyncBindingBehaviors.tspackages/bind/spec/bindingAttributeBehaviors.tspackages/bind/spec/bindingHandlerBehaviors.ts
✅ Files skipped from review due to trivial changes (1)
- builds/knockout/spec/dependentObservableBehaviors.js
🚧 Files skipped from review as they are similar to previous changes (1)
- builds/knockout/spec/defaultBindings/valueBehaviors.js
phillipc
reviewed
Apr 5, 2026
| "@types/jasmine": "^1.3.7", | ||
| "@types/jquery": "^4.0.0", | ||
| "@types/mocha": "^10.0.10", | ||
| "chai": "^4.5.0", |
Member
There was a problem hiding this comment.
We should also update chai and sinon in this pr
phillipc
reviewed
Apr 5, 2026
| } | ||
|
|
||
| function expectHaveValues(actual, expectedValues) { | ||
| const values = ko.utils.arrayFilter(ko.utils.arrayMap(actual.childNodes, function (node) { return node.value }), function (value) { return value !== undefined }) |
Member
There was a problem hiding this comment.
In theory, we test tko against tko. The assertions should not use tko again. Just a comment.
phillipc
reviewed
Apr 5, 2026
| expect(html).to.equal(normalizedExpectedHtml) | ||
| } | ||
|
|
||
| function expectContainText(actual, expectedText, ignoreSpaces) { |
Member
There was a problem hiding this comment.
Probably another way: https://www.chaijs.com/api/plugins/#method_addmethod
However, I don't think it's worse because it's less dependent on chai in the current way.
Member
|
@brianmhunt I would merge #287 in this pr and make a review. Okay for you? |
phillipc
reviewed
Apr 7, 2026
| @@ -1,12 +1,41 @@ | |||
| function cleanedHtml(node) { | |||
Member
There was a problem hiding this comment.
Redundant code move to helper class
phillipc
reviewed
Apr 7, 2026
| didResolveDefinition = true; | ||
| expect(definition.template).toBe(domNodeArray); | ||
| expect(definition.createViewModel).toBe(createViewModelFunction); | ||
| expect(definition.template).to.equal(domNodeArray); |
Member
There was a problem hiding this comment.
We could have written a ToBe extension for chai, of course, and it would have involved fewer changes. Oh well.
phillipc
reviewed
Apr 7, 2026
| it('Can be configured as the ID of a <script> element', function() { | ||
| // Special case: the script's text should be interpreted as a markup string | ||
| testTemplateFromElement('<script id="my-script-elem" type="text/html">{0}</script>', 'my-script-elem'); | ||
| return testTemplateFromElement('<script id="my-script-elem" type="text/html">{0}</script>', 'my-script-elem'); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
build.knockoutspecs from Jasmine-era test patterns to Mocha + Chai + SinonqueueMicrotasktask scheduler wrapperVerification
make tscmake test-headlessorigin/mainin/tmp/tko-mainusingmake test-headless, checking for assertion loss as packages were convertedbuilds/knockout: current branchTOTAL: 955 SUCCESS; clean mainTOTAL: 955 SUCCESSpackages/filter.punchescurrent branchTOTAL: 6 SUCCESS; clean mainTOTAL: 6 SUCCESSNotes
it.skip/describe.skipcases are pre-existing coverage gaps, not new skips introduced by this migrationbuild.knockoutno longer carriescreateSpy/Clockcompatibility shimsAGENTS.mdand README now reflect the Mocha/Chai/Sinon end stateSummary by CodeRabbit
Documentation
Tests
Chores