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

Move some App.test.tsx helpers to utils #2056

Merged
merged 4 commits into from
Jul 19, 2022
Merged

Move some App.test.tsx helpers to utils #2056

merged 4 commits into from
Jul 19, 2022

Conversation

wolmir
Copy link
Contributor

@wolmir wolmir commented Jul 18, 2022

Copy link
Contributor

@julieg18 julieg18 left a comment

Choose a reason for hiding this comment

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

Nice job, you've taken about 500 lines off of the file! I spotted a couple of typos, but overall, it looks good to me!

renderTable(sortingTableDataFixture)
}

export const renderTableWithoutRuningExperiments = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean renderTableWithoutRunningExperiments?

renderTable({ ...tableDataFixture, rows: [tableDataFixture.rows[0]] })
}

export const renderTableWithSortingdata = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean renderTableWithSortingData?

getCountIndicators,
renderTable,
renderTableWithNoColumns,
renderTableWithoutRuningExperiments,
Copy link
Member

Choose a reason for hiding this comment

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

Running

renderTableWithNoColumns,
renderTableWithoutRuningExperiments,
renderTableWithPlaceholder,
renderTableWithSortingdata,
Copy link
Member

Choose a reason for hiding this comment

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

SortingData

const tailRow = within(getRow('42b8736')).getByRole('checkbox')
fireEvent.click(tailRow, { shiftKey: true })
clickRowCheckbox('4fb124a')
clickRowCheckbox('42b8736', true)

const selectedRows = screen.getAllByRole('row', { selected: true })
Copy link
Member

Choose a reason for hiding this comment

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

[C] This could be a helper as well:

const selectedRows = (screen) => screen.getAllByRole('row', { selected: true })

Copy link
Member

Choose a reason for hiding this comment

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

or import screen into the utils file.

@@ -0,0 +1,87 @@
/* eslint-disable unicorn/filename-case */
Copy link
Contributor

Choose a reason for hiding this comment

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

In theory, it should already be in the ignore pattern of the rule (all files in test folders are ignored for PascalCase rule, but I think we had that because we used to have something like util.tsx. I'm torn between fixing the rule so that your file gets ignored and you can remove that line and just using PascalCase here (and even removing the ignore pattern on the test files).

Copy link
Contributor Author

@wolmir wolmir Jul 19, 2022

Choose a reason for hiding this comment

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

I forgot about this issue!
I prefer to ignore the rule for cases like this (not just tests).
My reason is that a PascalCase implies that the file is either a component (Like IconMenu.tsx) or a family of related components (like Row(s).tsx).
But for buildDynamicColumns.tsx and this file the jsx code is just ancillary stuff.
They are not the main purpose of the file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then it should be in the ignore pattern of the rule. It is already the case, but it must be broken.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add it, thanks!

@wolmir wolmir force-pushed the try-to-shrink-app-test branch from 6af4cda to e698bc9 Compare July 19, 2022 13:35
@codeclimate
Copy link

codeclimate bot commented Jul 19, 2022

Code Climate has analyzed commit 167cb08 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (85% is the threshold).

This pull request will bring the total coverage in the repository to 96.7% (0.0% change).

View more on Code Climate.

@wolmir wolmir merged commit 39ea7fe into main Jul 19, 2022
@wolmir wolmir deleted the try-to-shrink-app-test branch July 19, 2022 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants