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

test: [M3-7863] - Use happy-dom instead of jsdom in unit tests #11085

Merged
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
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
2 changes: 1 addition & 1 deletion packages/manager/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@
"eslint-plugin-xss": "^0.1.10",
"factory.ts": "^0.5.1",
"glob": "^10.3.1",
"jsdom": "^24.1.1",
"happy-dom": "^15.7.4",
"junit2json": "^3.1.4",
"lint-staged": "^15.2.9",
"mocha-junit-reporter": "^2.2.1",
Expand Down
6 changes: 3 additions & 3 deletions packages/manager/src/components/Avatar/Avatar.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ describe('Avatar', () => {
const avatarStyles = getComputedStyle(avatar);

expect(getByTestId('avatar-letter')).toHaveTextContent('M');
expect(avatarStyles.backgroundColor).toBe('rgb(1, 116, 188)'); // theme.color.primary.dark (#0174bc)
expect(avatarStyles.backgroundColor).toBe('#0174bc'); // theme.color.primary.dark (#0174bc)
});

it('should render a background color from props', () => {
Expand All @@ -48,8 +48,8 @@ describe('Avatar', () => {
const avatarTextStyles = getComputedStyle(avatarText);

// Confirm background color contrasts with text color.
expect(avatarStyles.backgroundColor).toBe('rgb(0, 0, 0)'); // black
expect(avatarTextStyles.color).toBe('rgb(255, 255, 255)'); // white
expect(avatarStyles.backgroundColor).toBe('#000000'); // black
expect(avatarTextStyles.color).toBe('#fff'); // white
});

it('should render the first letter of username from props', async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ describe('BetaChip', () => {
const { getByTestId } = renderWithTheme(<BetaChip color="primary" />);
const betaChip = getByTestId('betaChip');
expect(betaChip).toBeInTheDocument();
expect(betaChip).toHaveStyle('background-color: rgb(16, 138, 214)');
expect(betaChip).toHaveStyle('background-color: #108ad6');
});

it('triggers an onClick callback', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ describe('Description List', () => {
it('has it title bolded', () => {
const { getByText } = renderWithTheme(<DescriptionList items={items} />);
const title = getByText('Random title');
expect(title).toHaveStyle('font-family: "LatoWebBold",sans-serif');
expect(title).toHaveStyle('font-family: LatoWebBold, sans-serif');
});

it('renders a column by default', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,52 +4,52 @@ exports[`HighlightedMarkdown component > should highlight text consistently 1`]
<DocumentFragment>
<p
class="MuiTypography-root MuiTypography-body1 formatted-text undefined css-ma9s0e-MuiTypography-root"
/>
<h1>
Some markdown
</h1>

>
<h1>
Some markdown
</h1>

<pre>
<code
class="hljs javascript"
>
<span
class="hljs-keyword"
>
const
</span>
x =
<span
class="hljs-function"
<pre>
<code
class="hljs javascript"
>
<span
class="hljs-keyword"
>
function
const
</span>
(
x =
<span
class="hljs-params"
/>
)
</span>
{
<span
class="hljs-keyword"
>
return
</span>

<span
class="hljs-literal"
>
true
</span>
; }
class="hljs-function"
>
<span
class="hljs-keyword"
>
function
</span>
(
<span
class="hljs-params"
/>
)
</span>
{
<span
class="hljs-keyword"
>
return
</span>

<span
class="hljs-literal"
>
true
</span>
; }

</code>
</pre>
<p />
</code>
</pre>
</p>
</DocumentFragment>
`;
4 changes: 2 additions & 2 deletions packages/manager/src/components/Notice/Notice.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ describe('Notice Component', () => {
const notice = container.firstChild;

expect(notice).toHaveStyle('margin-bottom: 24px');
expect(notice).toHaveStyle('margin-left: 0');
expect(notice).toHaveStyle('margin-top: 0');
expect(notice).toHaveStyle('margin-left: 0px');
expect(notice).toHaveStyle('margin-top: 0px');
});

it('renders with text', () => {
Expand Down
2 changes: 1 addition & 1 deletion packages/manager/src/components/Tabs/Tab.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ describe('Tab Component', () => {

expect(tabElement).toHaveStyle(`
display: inline-flex;
color: rgb(0, 156, 222);
color: #0174bc;
`);
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ describe('TextTooltip', () => {

const displayText = getByText(props.displayText);

expect(displayText).toHaveStyle('color: rgb(0, 156, 222)');
expect(displayText).toHaveStyle('color: #0174bc');
expect(displayText).toHaveStyle('font-size: 18px');
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,9 @@ describe('Database Create', () => {
const { getAllByTestId, getAllByText } = renderWithTheme(
<DatabaseCreate />
);
await waitForElementToBeRemoved(getAllByTestId(loadingTestId));
await waitForElementToBeRemoved(getAllByTestId(loadingTestId), {
timeout: 10_000,
});

getAllByText('Cluster Label');
getAllByText('Database Engine');
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { screen, within } from '@testing-library/react';
import { screen, waitFor, within } from '@testing-library/react';
import { fireEvent } from '@testing-library/react';
import { waitForElementToBeRemoved } from '@testing-library/react';
import { DateTime } from 'luxon';
Expand Down Expand Up @@ -71,6 +71,7 @@ describe('Database Table Row', () => {

describe('Database Table', () => {
it('should render database landing table with items', async () => {
const database = databaseInstanceFactory.build({ status: 'active' });
const mockAccount = accountFactory.build({
capabilities: [managedDBBetaCapability],
});
Expand All @@ -81,32 +82,25 @@ describe('Database Table', () => {
);
server.use(
http.get(databaseInstancesEndpoint, () => {
const databases = databaseInstanceFactory.buildList(1, {
status: 'active',
});
return HttpResponse.json(makeResourcePage(databases));
return HttpResponse.json(makeResourcePage([database]));
})
);

const { getAllByText, getByTestId, queryAllByText } = renderWithTheme(
<DatabaseLanding />
);
const { getByText } = renderWithTheme(<DatabaseLanding />);

// Loading state should render
expect(getByTestId(loadingTestId)).toBeInTheDocument();

await waitForElementToBeRemoved(getByTestId(loadingTestId));
// wait for API data to load
await waitFor(() => expect(getByText(database.label)).toBeVisible(), {
timeout: 10_000,
});
Comment on lines +92 to +94
Copy link
Member Author

Choose a reason for hiding this comment

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

The is the only sketchy change, everything else when pretty smoothly

Copy link
Contributor

Choose a reason for hiding this comment

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

Have you tried awaiting a findByText?

Copy link
Member Author

@bnussman-akamai bnussman-akamai Oct 11, 2024

Choose a reason for hiding this comment

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

I did try that but the default timeout wasn't long enough πŸ˜– I might be able to revert to findByText but I think his accomplishes the same thing

expect(getByText('Active')).toBeVisible();

// Static text and table column headers
getAllByText('Cluster Label');
getAllByText('Status');
getAllByText('Configuration');
getAllByText('Engine');
getAllByText('Region');
getAllByText('Created');

// Check to see if the mocked API data rendered in the table
queryAllByText('Active');
expect(getByText('Cluster Label')).toBeVisible();
expect(getByText('Status')).toBeVisible();
expect(getByText('Configuration')).toBeVisible();
expect(getByText('Engine')).toBeVisible();
expect(getByText('Region')).toBeVisible();
expect(getByText('Created')).toBeVisible();
});

it('should render database landing with empty state', async () => {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { fireEvent } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
import * as React from 'react';

import { UNKNOWN_PRICE } from 'src/utilities/pricing/constants';
Expand Down Expand Up @@ -42,11 +42,11 @@ describe('HAControlPlane', () => {
await findByText(/\$60\.00/);
});

it('should call the handleChange function on change', () => {
it('should call the handleChange function on change', async () => {
const { getByTestId } = renderWithTheme(<HAControlPlane {...props} />);
const haRadioButton = getByTestId('ha-radio-button-yes');

fireEvent.click(haRadioButton);
await userEvent.click(haRadioButton);
expect(props.setHighAvailability).toHaveBeenCalled();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ describe('Disks', () => {
const { getByTestId } = render(wrapWithTheme(<Disks {...props} />));
disks.forEach((eachDisk) => {
const checkbox = getByTestId(`checkbox-${eachDisk.id}`).parentNode;
fireEvent.click(checkbox as any);
fireEvent.click(checkbox!);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also rewrite this as a userEvent?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ideally yes, but I want to keep this PR's scope as narrow as possible so it is representative of what was absolutely necessary to change to get happy-dom working

There are so many places where fireEvent is used when userEvent would be preferable 😣

expect(mockHandleSelect).toHaveBeenCalledWith(eachDisk.id);
});
});
Expand All @@ -47,10 +47,10 @@ describe('Disks', () => {
});

it('checks the disk if the associated config is selected', () => {
const { getByTestId } = render(
const { getByRole } = render(
wrapWithTheme(<Disks {...props} selectedConfigIds={[9859511]} />)
);
const checkbox: any = getByTestId('checkbox-19040624').firstElementChild;
expect(checkbox).toHaveAttribute('checked');
const checkbox = getByRole('checkbox', { name: '512 MB Swap Image' });
expect(checkbox).toBeChecked();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ describe('VPC', () => {

it('renders VPC IPv4, NAT checkboxes, and IP Ranges inputs when a subnet is selected', async () => {
const {
getByLabelText,
getByRole,
getByText,
} = renderWithThemeAndHookFormContext<CreateLinodeRequest>({
component: <VPC />,
Expand All @@ -82,21 +82,23 @@ describe('VPC', () => {
});

expect(
getByLabelText(
'Auto-assign a VPC IPv4 address for this Linode in the VPC'
)
getByRole('checkbox', {
name: 'Auto-assign a VPC IPv4 address for this Linode in the VPC',
})
).toBeInTheDocument();

expect(
getByLabelText('Assign a public IPv4 address for this Linode')
getByRole('checkbox', {
name: 'Assign a public IPv4 address for this Linode',
})
).toBeInTheDocument();

expect(getByText('Assign additional IPv4 ranges')).toBeInTheDocument();
});

it('should check the VPC IPv4 if a "ipv4.vpc" is null/undefined', async () => {
const {
getByLabelText,
getByRole,
} = renderWithThemeAndHookFormContext<CreateLinodeRequest>({
component: <VPC />,
useFormOptions: {
Expand All @@ -112,15 +114,16 @@ describe('VPC', () => {
});

expect(
getByLabelText(
'Auto-assign a VPC IPv4 address for this Linode in the VPC'
)
getByRole('checkbox', {
name: 'Auto-assign a VPC IPv4 address for this Linode in the VPC',
})
).toBeChecked();
});

it('should uncheck the VPC IPv4 if a "ipv4.vpc" is a string value and show the VPC IP TextField', async () => {
const {
getByLabelText,
getByRole,
} = renderWithThemeAndHookFormContext<CreateLinodeRequest>({
component: <VPC />,
useFormOptions: {
Expand All @@ -132,9 +135,9 @@ describe('VPC', () => {
});

expect(
getByLabelText(
'Auto-assign a VPC IPv4 address for this Linode in the VPC'
)
getByRole('checkbox', {
name: 'Auto-assign a VPC IPv4 address for this Linode in the VPC',
})
).not.toBeChecked();

expect(getByLabelText('VPC IPv4 (required)')).toBeVisible();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,10 @@ describe('Linode Create', () => {
});

it('Should not render the region select when creating from a backup', () => {
const { queryByText } = renderWithTheme(<LinodeCreate />, {
const { queryByLabelText } = renderWithTheme(<LinodeCreate />, {
MemoryRouter: { initialEntries: ['/linodes/create?type=Backups'] },
});

expect(queryByText('Region')).toBeNull();
expect(queryByLabelText('Region')).toBeNull();
});
});
Loading