test: Remove sinon clock dependency to unblock the CI pipeline#1624
test: Remove sinon clock dependency to unblock the CI pipeline#1624danieljbruce merged 9 commits intomainfrom
Conversation
| toFake: [ | ||
| 'setTimeout', | ||
| 'clearTimeout', | ||
| 'setImmediate', |
There was a problem hiding this comment.
It's possible you could just remove setImmediate and clearImmediate here (and also probably nextTick) and that would also fix it. I seem to remember having troubles with this same issue on Pub/Sub, now that you mention it, because they started hooking these by default.
There was a problem hiding this comment.
I had tried emptying this array earlier and it didn't work for some reason. Also, now I'm getting a strange error that says "Exception during run: SyntaxError[ @/Users/djbruce/Documents/Programming/nodejs-bigtable/build/system-test/app-profile.js ]: Unexpected token '||='" so I think I'll leave this as is to unblock the CI, but I am willing to revisit it.
There was a problem hiding this comment.
Here's a sample of this change at main...sinon-timers-2. The tests still seem to fail. I wonder what the actual issue with the timer really is and why it is preventing the emitted event from being received.
|
|
||
| tests.forEach(test => { | ||
| it(test.name, () => { | ||
| it(test.name, (done: mocha.Done) => { |
There was a problem hiding this comment.
If you want to avoid the extra mocha dependency, you can also just do done => { here. TS will figure it out.
There was a problem hiding this comment.
It's a good idea. I'll do this in a separate PR to unblock CI ASAP.
Description
Two days ago the system tests suddenly started failing on main even though no changes were made to the codebase. In the tests, the line of code that says clock.runAll(); would wait for all the events to be emitted and processed by the client library so that all the variables used in assertion checks would have the right values and the assertion checks would pass. Suddenly the clock.runAll(); line of code doesn't wait for the events to be processed by the client library so the assertion checks fail. It also seems that even when the assertion checks are omitted the test passes, but the events still don't get processed.
The test framework is changed so that we don't rely on the sinon clock dependency anymore because an update to this dependency seems to have broke the test. Instead, we do all the assertion checks when an error or end event passes through the client library and reaches the test runner since this is when we were doing the assertion checks before anyway when the sinon clock was working properly.
Impact
Now system tests will pass so we can develop on nodejs-bigtable again.
Testing
Modified a test. This doesn't reduce test coverage. It just removes the dependency on the sinon clock.
Additional Information
Before these changes 9 of the tests would fail. The eventemitter was emitting the events, but for some reason the events were never reaching the chunk transformer in the client library.