-
Notifications
You must be signed in to change notification settings - Fork 115
Add React Unit Tests for ColumnSummaryCell Component #9090
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.
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
Add React Unit Tests for ColumnSummaryCell Component #9090
Conversation
Simplifies test setup if we replace instance used from the context provider with the one from the prop - which are both the same
E2E Tests 🚀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great to see our first (I think) Positron React unit test! 🎉
As requested, my thoughts re mocking:
-
Should we type our mocks?
That way we get TS errors if we change the class/interface and forget to update the test and mocks. I've found that useful when refactoring. We also get better completions etc from LLMs and language servers.
Unfortunately AFAIK this would also require extracting more interfaces so there is a tradeoff.
-
I try to avoid Sinon where possible and just implement it in TS.
This tends to give more informative and accurate TS errors while refactoring as well as more reusable mocks if we end up sharing them across test files. It doesn't use too many more lines of code.
For example:
const mockHoverManager: IHoverManager = { hideHover: () => { }, showHover: () => { }, };
or
class MockHoverManager implements IHoverManager { hideHover(): void { } showHover(target: HTMLElement, content?: string | (() => string | undefined)): void { } }
-
I typically reserve Sinon for when I absolutely need to:
- When I have to overwrite some method or property and can't pass in a mocked implemention e.g.
stub()
bing parts of thevscode
andpositron
extension APIs in extension tests. - When my test requires spying and making assertions about function/method calls.
- When I have to overwrite some method or property and can't pass in a mocked implemention e.g.
-
Should we use
act
instead ofsetTimeout(..., 0)
? Some other possibly interesting React testing ideas here: https://react.dev/reference/react/act
Once we agree on our preferences, it might be worth collecting them in the wiki for the rest of the team.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are great and work well.
I will defer judgement on mocking patterns to @seeM as he's got much more thought out opinions there than I do.
I made a note about adding a helper function for waiting for an element to appear rather than using the timeout trick.
Ideally the act()
function would work but I dont think we can use that here because the react we bundle with Positron is a static-prebuilt script instead of part of the larger build process. act()
is disabled in "production" react builds.
In the future we'll obviously break out some of this into testing utilities but for now I think this is a great foundation!
// Wait for initial render | ||
await new Promise(resolve => setTimeout(resolve, 0)); | ||
|
||
const nullPercentElement = container.querySelector('.text-percent'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Example of waiting function
const nullPercentElement = container.querySelector('.text-percent'); | |
const nullPercentElement = await waitForElement(container, '.text-percent'); |
|
||
return mockInstance as unknown as TableSummaryDataGridInstance; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could get playwright-esque behavior for rendering this way as well. Maybe a bit too heavy handed though.
/** | |
* Waits for an element to appear in the DOM. | |
* @param container The container element to search within | |
* @param selector The CSS selector for the element to wait for | |
* @param timeout Maximum time to wait in milliseconds (default: 100ms) | |
* @returns Promise that resolves with the found element or rejects if timeout | |
*/ | |
function waitForElement( | |
container: HTMLElement, | |
selector: string, | |
timeout = 250 | |
): Promise<Element> { | |
return new Promise((resolve, reject) => { | |
// Check if element already exists | |
const element = container.querySelector(selector); | |
if (element) { | |
return resolve(element); | |
} | |
// Set up mutation observer to watch for the element | |
const observer = new MutationObserver(() => { | |
const el = container.querySelector(selector); | |
if (el) { | |
observer.disconnect(); | |
resolve(el); | |
} | |
}); | |
// Start observing DOM changes | |
observer.observe(container, { | |
// Check for changes in attributes in case the waited for element | |
// exists but has the selector added after the element is created | |
attributes: true, | |
childList: true, | |
subtree: true, | |
}); | |
// Set up timeout to prevent hanging tests | |
setTimeout(() => { | |
observer.disconnect(); | |
reject( | |
new Error( | |
`Timed out after ${timeout}ms waiting for element: ${selector}` | |
) | |
); | |
}, timeout); | |
}); | |
} |
Looks great! |
This PR adds a React unit test for the bug from #8515
The unit test is for the
ColumnSummaryCell
component, that verifies the null percentage display value from the Data Explorer Summary panel matches the expectations in the UI.The test checks 4 specific null percentage formatting edge cases:
You can run the unit test in several different ways:
The react unit test setup uses Mocha + assert + Sinon.js + React instead of the now common place react-testing-library. This is to reduce introducing any additional dependencies since the number of component unit tests will be added on an as-needed basis.
ReactDOM.createRoot()
container.querySelector
)assert
andsinon
for test logic (vs RTL querying and jest mocking)Release Notes
New Features
Bug Fixes
QA Notes
@:data-explorer
@:web
@:win