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.

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

app.test.tsx changes #21

Merged
merged 4 commits into from
Sep 25, 2024
Merged

Conversation

mbondyra
Copy link

Summary

Summarize your PR. If it involves visual changes include a screenshot or gif.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@mbondyra mbondyra force-pushed the app.test.tsx branch 4 times, most recently from 2d22e8e to cb923f5 Compare September 19, 2024 20:12
Copy link
Owner

@dej611 dej611 left a comment

Choose a reason for hiding this comment

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

Tested locally and works great.
Noticed the Provider error messages but I think we can work around those later on.

function makeDefaultProps(): jest.Mocked<LensAppProps> {
return {
editorFrame: createMockFrame(),
let props: jest.Mocked<LensAppProps>;
Copy link
Owner

Choose a reason for hiding this comment

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

One question here.
What's the benefit of having a shared variable and reset it on every test with a beforeEach vs generating all the mocks explicitly every time?
It is just code readability (not calling it every time) or there's more?

Copy link
Author

Choose a reason for hiding this comment

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

There's way less code to read (like calling makeDefaultProps function and then modifying the properties we want and then pass it to the renderApp) with this pattern so I think it's just nicer :)


const sessionIdSubject = new Subject<string>();
const getLensDocumentMock = (someProps?: Partial<LensDocument>) => ({
Copy link
Owner

Choose a reason for hiding this comment

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

In the original code there's a cloneDeep function that wraps this to enable document manipulation within tests without worry about influencing other tests.

Copy link
Author

Choose a reason for hiding this comment

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

reverted 👌🏼

@dej611 dej611 merged commit e9a6508 into dej611:feat/react-embeddable Sep 25, 2024
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.

2 participants