Skip to content

remove enzyme code studio and replace with react-testing-library#565

Merged
Zhou-Ziheng merged 8 commits intodeephaven:mainfrom
Zhou-Ziheng:562-remove-enzyme-code-studio
May 11, 2022
Merged

remove enzyme code studio and replace with react-testing-library#565
Zhou-Ziheng merged 8 commits intodeephaven:mainfrom
Zhou-Ziheng:562-remove-enzyme-code-studio

Conversation

@Zhou-Ziheng
Copy link
Contributor

Fixes #562

@Zhou-Ziheng Zhou-Ziheng self-assigned this May 10, 2022
@Zhou-Ziheng Zhou-Ziheng added the enhancement New feature or request label May 10, 2022
@codecov
Copy link

codecov bot commented May 10, 2022

Codecov Report

Merging #565 (181ce07) into main (ecfc74a) will increase coverage by 0.25%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #565      +/-   ##
==========================================
+ Coverage   36.43%   36.68%   +0.25%     
==========================================
  Files         390      390              
  Lines       28642    28655      +13     
  Branches     6777     6775       -2     
==========================================
+ Hits        10436    10513      +77     
+ Misses      17860    17796      -64     
  Partials      346      346              
Flag Coverage Δ
unit 36.68% <ø> (+0.25%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/code-studio/src/main/AppMainContainer.jsx 33.60% <ø> (+0.40%) ⬆️
packages/console/src/csv/CsvInputBar.jsx 3.84% <0.00%> (-0.13%) ⬇️
packages/console/src/Console.jsx 17.03% <0.00%> (ø)
...dashboard-core-plugins/src/panels/ConsolePanel.jsx 5.59% <0.00%> (ø)
...kages/dashboard-core-plugins/src/ConsolePlugin.tsx 21.10% <0.00%> (+1.10%) ⬆️
packages/code-studio/src/main/AppControlsMenu.jsx 18.03% <0.00%> (+6.55%) ⬆️
packages/console/src/csv/CsvOverlay.jsx 40.70% <0.00%> (+35.25%) ⬆️
packages/code-studio/src/main/WidgetList.tsx 49.18% <0.00%> (+45.90%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ecfc74a...181ce07. Read the comment docs.

@Zhou-Ziheng Zhou-Ziheng requested a review from mofojed May 10, 2022 17:43
Comment on lines +94 to +102
// jest.mock('@deephaven/components', () => ({
// ...jest.requireActual('@deephaven/components'),
// __esModule: true,
// Popper: jest.fn(({ children }) => {
// return children;
// }),
// default: jest.fn(),
// }));

Copy link
Member

Choose a reason for hiding this comment

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

Delete unused

Suggested change
// jest.mock('@deephaven/components', () => ({
// ...jest.requireActual('@deephaven/components'),
// __esModule: true,
// Popper: jest.fn(({ children }) => {
// return children;
// }),
// default: jest.fn(),
// }));

Comment on lines +81 to +92
jest.mock('@deephaven/dashboard', () => ({
...jest.requireActual('@deephaven/dashboard'),
__esModule: true,
Dashboard: jest.fn(({ hydrate }) => {
const result = hydrate(mockProp, mockId);
if (result.fetch) {
result.fetch();
}
return <p>{JSON.stringify(result)}</p>;
}),
default: jest.fn(),
}));
Copy link
Member

Choose a reason for hiding this comment

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

The reason why you were still getting the mockConstructor error here is because you're only passing jest.fn() here for default. You could keep the default import and then just set default instead of Dashboard here, e.g.:

Suggested change
jest.mock('@deephaven/dashboard', () => ({
...jest.requireActual('@deephaven/dashboard'),
__esModule: true,
Dashboard: jest.fn(({ hydrate }) => {
const result = hydrate(mockProp, mockId);
if (result.fetch) {
result.fetch();
}
return <p>{JSON.stringify(result)}</p>;
}),
default: jest.fn(),
}));
jest.mock('@deephaven/dashboard', () => ({
...jest.requireActual('@deephaven/dashboard'),
__esModule: true,
default: jest.fn(({ hydrate }) => {
const result = hydrate(mockProp, mockId);
if (result.fetch) {
result.fetch();
}
return <p>{JSON.stringify(result)}</p>;
}),
}));

Change the import in AppMainContainer is also acceptable, but then you don't need to have the __esModule: true, default: jest.fn() bit here.

plugins = new Map(),
} = {}) {
return shallow(
return (
Copy link
Member

Choose a reason for hiding this comment

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

If you kept this method as renderAppMainContainer, and then do a return render(<AppMainContainer ... />) here, then you can just call renderAppMainContainer() in all the tests below without having to wrap it in render each time like render(getAppMainContainer()). Not a big deal either way.

})
);
expect(result).not.toEqual(expect.objectContaining({ fetch: expect.any }));
session = makeSession();
Copy link
Member

Choose a reason for hiding this comment

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

You could still keep the beforeEach block with the makeSession in it, as it makes it clear the session is always initialized to the same thing.

@Zhou-Ziheng Zhou-Ziheng requested a review from mofojed May 11, 2022 13:06
@Zhou-Ziheng Zhou-Ziheng merged commit 7418086 into deephaven:main May 11, 2022
@github-actions github-actions bot locked and limited conversation to collaborators May 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove enzyme from code-studio (AppMainContainer, FormattingSectionContent)

2 participants