Skip to content

Conversation

pawelangelow
Copy link
Collaborator

rejson slice is hard to test due to its architecture. As a temporary solution, I added a new test suite, where modules can be dynamically reloaded in order to use the jest mocks. Ideally, the business logic from this slice should be extracted and properly tested.

Apart from the event properties, this CL fixes a bug with reporting the JSON path level of nesting.

dantovska
dantovska previously approved these changes May 5, 2025
KrumTy
KrumTy previously approved these changes May 7, 2025
KIvanow
KIvanow previously approved these changes May 7, 2025
import { AppDispatch, RootState } from '../store'

export const JSON_LENGTH_TO_FORCE_RETRIEVE = 200
const TELEMETRY_KEY_LEVEL_WHOLE_KEY = 'entireKey'
Copy link
Collaborator

Choose a reason for hiding this comment

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

key is named ..._WHOLE_KEY the value entireKey. Why not ..._ENTIRE_KEY

Comment on lines +9 to +22
// Suppress Redux warnings about missing reducers
beforeAll(() => {
console.error = (...args: any[]) => {
const message = args[0]
if (
typeof message === 'string' &&
message.includes('No reducer provided for key')
) {
return
}

originalConsoleError(...args)
}
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why for this test specifically?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a brand new test suite, and I'm adding it here. I'm suppressing the expected Redux warnings to keep the logs clean and free of noise.

}
})

const rejson = await import('uiSrc/slices/browser/rejson')
Copy link
Collaborator

Choose a reason for hiding this comment

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

why the dynamic imports?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The mocking process for this file is a bit tricky. It seems something gets imported before Jest has a chance to apply the usual jest.mock() logic, so the standard mocking approach doesn’t work here. That’s why this file uses jest.resetModules() - to force a clean module state.

}

const getJsonPathLevel = (path: string): string => {
const getJsonPathLevel = (path: string): number => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think some tests verifying this funciton works as expected won't hurt

@pawelangelow pawelangelow dismissed stale reviews from KIvanow, KrumTy, and dantovska via 5b2587f May 13, 2025 15:27
@pawelangelow pawelangelow merged commit 459b72b into main May 14, 2025
28 checks passed
@pawelangelow pawelangelow deleted the feature/RI-6953-add-more-telemetry-properties branch May 14, 2025 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants