fix: some others flaky tests (#1808)

* fix: some others flaky tests

* fix: minor refactor of timers

* style: remove useless console.log

Author: robertsLando

src/lib/PingTimer.ts

@@ -1,5 +1,4 @@
-import { clearTimeout as clearT, setTimeout as setT } from 'worker-timers'
-import isBrowser, { isWebWorker } from './is-browser'
+import timers from './timers'
 
 export default class PingTimer {
 	private keepalive: number
@@ -8,16 +7,6 @@ export default class PingTimer {
 
 	private checkPing: () => void
 
-	// dont directly assign globals to class props otherwise this throws in web workers: Uncaught TypeError: Illegal invocation
-	// See: https://stackoverflow.com/questions/9677985/uncaught-typeerror-illegal-invocation-in-chrome
-	private _setTimeout: typeof setT =
-		isBrowser && !isWebWorker
-			? setT
-			: (func, time) => setTimeout(func, time)
-
-	private _clearTimeout: typeof clearT =
-		isBrowser && !isWebWorker ? clearT : (timer) => clearTimeout(timer)
-
 	constructor(keepalive: number, checkPing: () => void) {
 		this.keepalive = keepalive * 1000
 		this.checkPing = checkPing
@@ -26,14 +15,14 @@ export default class PingTimer {
 
 	clear() {
 		if (this.timer) {
-			this._clearTimeout(this.timer)
+			timers.clear(this.timer)
 			this.timer = null
 		}
 	}
 
 	reschedule() {
 		this.clear()
-		this.timer = this._setTimeout(() => {
+		this.timer = timers.set(() => {
 			this.checkPing()
 			// prevent possible race condition where the timer is destroyed on _cleauUp
 			// and recreated here

src/lib/timers.ts

@@ -0,0 +1,15 @@
+import isBrowser, { isWebWorker } from './is-browser'
+import { clearTimeout as clearT, setTimeout as setT } from 'worker-timers'
+
+// dont directly assign globals to class props otherwise this throws in web workers: Uncaught TypeError: Illegal invocation
+// See: https://stackoverflow.com/questions/9677985/uncaught-typeerror-illegal-invocation-in-chrome
+
+const timers: { set: typeof setT; clear: typeof clearT } = {
+	set:
+		isBrowser && !isWebWorker
+			? setT
+			: (func, time) => setTimeout(func, time),
+	clear: isBrowser && !isWebWorker ? clearT : (timer) => clearTimeout(timer),
+}
+
+export default timers

test/abstract_client.ts

@@ -21,7 +21,6 @@ import { IPublishPacket, IPubrelPacket, ISubackPacket, QoS } from 'mqtt-packet'
 import { DoneCallback, ErrorWithReasonCode } from 'src/lib/shared'
 import { fail } from 'assert'
 import { describe, it, beforeEach, afterEach } from 'node:test'
-import { nextTick } from 'process'
 
 /**
  * These tests try to be consistent with names for servers (brokers) and clients,
@@ -44,6 +43,10 @@ import { nextTick } from 'process'
  *
  */
 
+const fakeTimersOptions = {
+	shouldClearNativeTimers: true,
+}
+
 export default function abstractTest(server, config, ports) {
 	const version = config.protocolVersion || 4
 
@@ -1963,7 +1966,7 @@ export default function abstractTest(server, config, ports) {
 
 		// eslint-disable-next-line
 		beforeEach(() => {
-			clock = sinon.useFakeTimers()
+			clock = sinon.useFakeTimers(fakeTimersOptions)
 		})
 
 		afterEach(() => {
@@ -2054,7 +2057,7 @@ export default function abstractTest(server, config, ports) {
 				timeout: 10000,
 			},
 			function _test(t, done) {
-				const clock = sinon.useFakeTimers()
+				const clock = sinon.useFakeTimers(fakeTimersOptions)
 
 				t.after(() => {
 					clock.restore()
@@ -2076,6 +2079,7 @@ export default function abstractTest(server, config, ports) {
 						assert.equal(err.message, 'Keepalive timeout')
 						client.once('connect', () => {
 							client.end(true, done)
+							clock.tick(100)
 							client = null
 						})
 					})
@@ -2094,7 +2098,7 @@ export default function abstractTest(server, config, ports) {
 			'should not reconnect if pingresp is successful',
 			{ timeout: 1000 },
 			function _test(t, done) {
-				const clock = sinon.useFakeTimers()
+				const clock = sinon.useFakeTimers(fakeTimersOptions)
 
 				t.after(() => {
 					clock.restore()
@@ -2118,6 +2122,7 @@ export default function abstractTest(server, config, ports) {
 						if (packet.cmd === 'pingreq') {
 							client.removeAllListeners('close')
 							client.end(true, done)
+							clock.tick(100)
 							client = null
 						}
 					})
@@ -2910,7 +2915,7 @@ export default function abstractTest(server, config, ports) {
 		})
 
 		it('should reconnect after stream disconnect', function _test(t, done) {
-			const clock = sinon.useFakeTimers()
+			const clock = sinon.useFakeTimers(fakeTimersOptions)
 
 			t.after(() => {
 				clock.restore()
@@ -2929,12 +2934,13 @@ export default function abstractTest(server, config, ports) {
 					tryReconnect = false
 				} else {
 					client.end(true, done)
+					clock.tick(100)
 				}
 			})
 		})
 
 		it("should emit 'reconnect' when reconnecting", function _test(t, done) {
-			const clock = sinon.useFakeTimers()
+			const clock = sinon.useFakeTimers(fakeTimersOptions)
 
 			t.after(() => {
 				clock.restore()
@@ -2960,12 +2966,13 @@ export default function abstractTest(server, config, ports) {
 				} else {
 					assert.isTrue(reconnectEvent)
 					client.end(true, done)
+					clock.tick(100)
 				}
 			})
 		})
 
 		it("should emit 'offline' after going offline", function _test(t, done) {
-			const clock = sinon.useFakeTimers()
+			const clock = sinon.useFakeTimers(fakeTimersOptions)
 
 			t.after(() => {
 				clock.restore()
@@ -2991,6 +2998,7 @@ export default function abstractTest(server, config, ports) {
 				} else {
 					assert.isTrue(offlineEvent)
 					client.end(true, done)
+					clock.tick(100)
 				}
 			})
 		})
@@ -3030,7 +3038,7 @@ export default function abstractTest(server, config, ports) {
 					timeout: 10000,
 				},
 				function _test(t, done) {
-					const clock = sinon.useFakeTimers()
+					const clock = sinon.useFakeTimers(fakeTimersOptions)
 
 					t.after(() => {
 						clock.restore()
@@ -3070,6 +3078,7 @@ export default function abstractTest(server, config, ports) {
 									)
 								}
 							})
+							clock.tick(100)
 						}
 					})
 				},
@@ -3084,8 +3093,8 @@ export default function abstractTest(server, config, ports) {
 			})
 			// bind client.end so that when it is called it is automatically passed in the done callback
 			setTimeout(() => {
-				client.end.call(client, done)
-			}, 50)
+				client.end(done)
+			}, 100)
 		})
 
 		it('should emit connack timeout error', function _test(t, done) {

test/client_mqtt5.ts

@@ -702,10 +702,8 @@ describe('MQTT 5.0', () => {
 				protocolVersion: 5,
 				properties: { authenticationData: Buffer.from([1, 2, 3, 4]) },
 			}
-			console.log('client connecting')
 			const client = mqtt.connect(opts)
 			client.on('error', (error) => {
-				console.log('error hit')
 				assert.strictEqual(
 					error.message,
 					'Packet has no Authentication Method',