search

ckeditor / ckeditor5

Powerful rich text editor framework with a modular architecture, modern integrations, and features like collaborative editing.
https://ckeditor.com/ckeditor-5
Other
9.5k stars 3.7k forks source link

Bump `sinon` to the latest version #11487

Closed psmyrek closed 1 month ago

psmyrek commented 2 years ago

Currently, we are using sinon in 9.x version in ckeditor5-dev-tests. Current latest version of sinon is 13.x, so it would be good to bump it.

However, there might be a few challenges (as we already tried it once quickly).

Executing yarn run test -f engine,utils --production fails:

  1. Error in DowncastDispatcher:

    Chrome 99.0.4844.74 (Windows 10) DowncastDispatcher convertChanges should re-render markers which view elements got unbound during conversion FAILED
    CKEditorError: model-nodelist-offset-out-of-bounds {"offset":3,"nodeList":[]}
    Read more: https://ckeditor.com/docs/ckeditor5/latest/framework/guides/support/error-codes.html#error-model-nodelist-offset-out-of-bounds
        at NodeList.offsetToIndex (./packages/ckeditor5-engine/src/model/nodelist.js:167:10)
        at RootElement.offsetToIndex (./packages/ckeditor5-engine/src/model/element.js:195:25)
        at getTextNodeAtPosition (./packages/ckeditor5-engine/src/model/position.js:1111:55)
        at TreeWalker._next (./packages/ckeditor5-engine/src/model/treewalker.js:241:94)
        at TreeWalker.next (./packages/ckeditor5-engine/src/model/treewalker.js:209:16)
        at Range.[Symbol.iterator] (./packages/ckeditor5-engine/src/model/range.js:78:10)
        at Generator.next (<anonymous>)
        at Function.from (<anonymous>)
        at deepEqual (base-dev/node_modules/sinon/pkg/sinon.js?ddbe64e0d0bd221ba7ec38c0c68d29e57c5f0132:7744:37)
        at deepEqualCyclic (base-dev/node_modules/sinon/pkg/sinon.js?ddbe64e0d0bd221ba7ec38c0c68d29e57c5f0132:7823:7)
    • Commenting out this line and this line helps, and then this test passes. It looks like the calledWith() assertion changes/triggers something in the model (?). There are some changes in the calledWith() assertion in sinon since v12 - see this commit.

  2. Error with fake timers.

    Chrome 99.0.4844.74 (Windows 10) SelectionObserver should not be treated as an infinite loop if changes are not often FAILED
    AssertError: expected warn to not have been called but was called once
        warn('FakeTimers: clearInterval was invoked to clear a native timer instead of one created by this library.\nTo automatically clean-up native timers, use `shouldClearNativeTimers`.') at http://localhost:9876/base-dev/node_modules/sinon/pkg/sinon.js?ddbe64e0d0bd221ba7ec38c0c68d29e57c5f0132:5900:33
        at Object.fail (base-dev/node_modules/sinon/pkg/sinon.js?ddbe64e0d0bd221ba7ec38c0c68d29e57c5f0132:174:25)
        at failAssertion (base-dev/node_modules/sinon/pkg/sinon.js?ddbe64e0d0bd221ba7ec38c0c68d29e57c5f0132:120:20)
        at Object.assert.<computed> [as notCalled] (base-dev/node_modules/sinon/pkg/sinon.js?ddbe64e0d0bd221ba7ec38c0c68d29e57c5f0132:149:17)
        at eval (./packages/ckeditor5-engine/tests/view/observer/selectionobserver.js:234:18)
    • The --production flag prevents from using any method from the console. Bumping the sinon package introduces also the new version of fake-timers package, which now requires to explicitly set shouldClearNativeTimers: true - see the docs. Otherwise, fake-timers prints the warning in the console, which is forbidden when our tests are executed with the --production flag.

  3. Stubbing non-writable properties.

    Chrome 99.0.4844.74 (Windows 10) getOptimalPosition() for single position should return coordinates (window scroll) FAILED
    TypeError: Descriptor for data property scrollX is non-writable
        at assertValidPropertyDescriptor (base-dev/node_modules/sinon/pkg/sinon.js?ddbe64e0d0bd221ba7ec38c0c68d29e57c5f0132:3785:15)
        at Function.stub (base-dev/node_modules/sinon/pkg/sinon.js?ddbe64e0d0bd221ba7ec38c0c68d29e57c5f0132:3702:5)
        at Sandbox.stub (base-dev/node_modules/sinon/pkg/sinon.js?ddbe64e0d0bd221ba7ec38c0c68d29e57c5f0132:3190:37)
        at stubWindow (./packages/ckeditor5-utils/tests/dom/position.js:773:92)
        at Context.eval (./packages/ckeditor5-utils/tests/dom/position.js:214:4)
    Chrome 99.0.4844.74 (Windows 10) getOptimalPosition() for single position positioned element parent should return coordinates FAILED
    TypeError: Descriptor for data property scrollX is non-writable
        at assertValidPropertyDescriptor (base-dev/node_modules/sinon/pkg/sinon.js?ddbe64e0d0bd221ba7ec38c0c68d29e57c5f0132:3785:15)
        at Function.stub (base-dev/node_modules/sinon/pkg/sinon.js?ddbe64e0d0bd221ba7ec38c0c68d29e57c5f0132:3702:5)
        at Sandbox.stub (base-dev/node_modules/sinon/pkg/sinon.js?ddbe64e0d0bd221ba7ec38c0c68d29e57c5f0132:3190:37)
        at stubWindow (./packages/ckeditor5-utils/tests/dom/position.js:773:92)
        at Context.eval (./packages/ckeditor5-utils/tests/dom/position.js:236:5)
    • Since v13, sinon does not allow stubbing non-configurable or non-writable properties - see this commit. Previously, it just silently failed, but now it throws an error.
psmyrek commented 1 year ago

I checked again with version 15.2.0 and in general the problems are the same.

  1. Some single calls to spy.calledWith() cause the editor to throw an error: CKEditorError: model-nodelist-offset-out-of-bounds.
    • No idea why this is happening. It would be easier to debug it if all spy.calledWith() calls behaved in the same way.
    • It seems there is a workaround: in tests that throw CKEditor error, instead of expect( spy.calledWith() ) we can use sinon.assert.calledWith( spy ) and these tests pass.
  2. Problem with clearing a native timer through fake timers: this can be easily fixed by initializing fake timers with the following configuration option: sinon.useFakeTimers( { shouldClearNativeTimers: true } ).
  3. Another problem with fake timers: Can't install fake timers twice on the same global object. This is actually a problem with the test itself, because fake timers are initialized more than once. In an earlier version of sinon such usage was silently ignored, but now it throws an error.

I haven't noticed any problems with stubbing non-configurable or non-writable properties. Maybe some tests have changed a bit in the meantime and there are no such tests anymore.

In conclusion, it seems that we can try to upgrade sinon to the latest version.

CKEditorBot commented 2 months ago

There has been no activity on this issue for the past year. We've marked it as stale and will close it in 30 days. We understand it may still be relevant, so if you're interested in the solution, leave a comment or reaction under this issue.

CKEditorBot commented 1 month ago

We've closed your issue due to inactivity. We understand that the issue may still be relevant. If so, feel free to open a new one (and link this issue to it).