Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✅ Fix test cleanup tasks order #3141

Merged
merged 2 commits into from
Nov 18, 2024
Merged

✅ Fix test cleanup tasks order #3141

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a90a585
Fix test cleanup tasks order
amortemousque Nov 18, 2024
4480edc
Fix wrong update
amortemousque Nov 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions packages/core/src/tools/instrumentMethod.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,11 +204,10 @@ describe('instrumentSetter', () => {

beforeEach(() => {
clock = mockClock()
zoneJs = mockZoneJs()

registerCleanupTask(() => {
clock.cleanup()
})
zoneJs = mockZoneJs()
})

it('replaces the original setter', () => {
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/tools/timer.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,11 @@ import { noop } from './utils/functionUtils'

beforeEach(() => {
clock = mockClock()
zoneJs = mockZoneJs()
registerCleanupTask(() => {
clock.cleanup()
resetMonitor()
})
zoneJs = mockZoneJs()
})

it('executes the callback asynchronously', () => {
Expand Down
12 changes: 10 additions & 2 deletions packages/core/test/emulate/mockReportingObserver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,16 @@ export function mockReportingObserver() {
export type MockCspEventListener = ReturnType<typeof mockCspEventListener>

export function mockCspEventListener() {
spyOn(document, 'addEventListener').and.callFake((_type: string, listener: EventListener) => {
listeners.push(listener)
// eslint-disable-next-line @typescript-eslint/unbound-method
const originalAddEventListener = EventTarget.prototype.addEventListener
EventTarget.prototype.addEventListener = jasmine
.createSpy()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems unrelated to this PR, but in any case:

❓ question: ‏spyOn(EventTarget.prototype, 'addEventListener') didn't work?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found cases where mocking the same API both manually with registerCleanup and with spyOn case don't work. I can't really explain why, but it seems reasonable to use the same mocking strategy twice. wdyt?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wdyt?

I guess spies are removed after all afterEach, so we have an order issue similar to what you are fixing in this ticket. So, I guess it makes sense.

Your explanation makes sense but it's a bit annoying that we cannot use Jasmine tools for that. This sounds extremely error prone..

One workaround I have in mind is to always attach spies first, before any other instrumentation. Then they will be detached after everything, so it will be fine.

An alternative could be to have our own spyOn utility that works with registerCleanupTask and forbid the jasmine one.

.and.callFake((_type: string, listener: EventListener) => {
listeners.push(listener)
})

registerCleanupTask(() => {
EventTarget.prototype.addEventListener = originalAddEventListener
})

const listeners: EventListener[] = []
Expand Down
25 changes: 13 additions & 12 deletions packages/core/test/leakDetection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,21 @@
import { display } from '../src/tools/display'
import { isIE } from '../src/tools/utils/browserDetection'
import { getCurrentJasmineSpec } from './getCurrentJasmineSpec'

let originalAddEventListener: typeof EventTarget.prototype.addEventListener
let originalRemoveEventListener: typeof EventTarget.prototype.removeEventListener
let wrappedListeners: {
[key: string]: Map<EventListenerOrEventListenerObject | null, EventListenerOrEventListenerObject | null>
}
import { registerCleanupTask } from './registerCleanupTask'

export function startLeakDetection() {
if (isIE()) {
return
}
wrappedListeners = {}

let wrappedListeners: {
[key: string]: Map<EventListenerOrEventListenerObject | null, EventListenerOrEventListenerObject | null>
} = {}

// eslint-disable-next-line @typescript-eslint/unbound-method
originalAddEventListener = EventTarget.prototype.addEventListener
const originalAddEventListener = EventTarget.prototype.addEventListener
// eslint-disable-next-line @typescript-eslint/unbound-method
originalRemoveEventListener = EventTarget.prototype.removeEventListener
const originalRemoveEventListener = EventTarget.prototype.removeEventListener

EventTarget.prototype.addEventListener = function (event, listener, options) {
if (!wrappedListeners[event]) {
Expand All @@ -32,15 +30,18 @@ export function startLeakDetection() {
wrappedListeners[event]?.delete(listener)
return originalRemoveEventListener.call(this, event, wrappedListener || listener, options)
}

registerCleanupTask(() => {
EventTarget.prototype.addEventListener = originalAddEventListener
EventTarget.prototype.removeEventListener = originalRemoveEventListener
wrappedListeners = {}
})
}

export function stopLeakDetection() {
if (isIE()) {
return
}
EventTarget.prototype.addEventListener = originalAddEventListener
EventTarget.prototype.removeEventListener = originalRemoveEventListener
wrappedListeners = {}
}

function withLeakDetection(eventName: string, listener: EventListener) {
Expand Down
2 changes: 1 addition & 1 deletion packages/core/test/registerCleanupTask.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
const cleanupTasks: Array<() => void> = []

export function registerCleanupTask(task: () => void) {
cleanupTasks.push(task)
cleanupTasks.unshift(task)
}

afterEach(() => {
Expand Down
12 changes: 6 additions & 6 deletions packages/rum-core/src/domain/error/trackReportError.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
mockClock,
mockCspEventListener,
mockReportingObserver,
registerCleanupTask,
} from '@datadog/browser-core/test'
import { mockRumConfiguration } from '../../../test'
import type { RumConfiguration } from '../configuration'
Expand All @@ -27,13 +28,12 @@ describe('trackReportError', () => {
notifyLog = jasmine.createSpy('notifyLog')
reportingObserver = mockReportingObserver()
subscription = errorObservable.subscribe(notifyLog)
cspEventListener = mockCspEventListener()
clock = mockClock()
})

afterEach(() => {
subscription.unsubscribe()
clock.cleanup()
registerCleanupTask(() => {
subscription.unsubscribe()
clock.cleanup()
})
cspEventListener = mockCspEventListener()
})

it('should track reports', () => {
Expand Down