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

Try out test helper #193

Merged
merged 19 commits into from
Jun 24, 2024
Merged

Try out test helper #193

merged 19 commits into from
Jun 24, 2024

Conversation

matthijsgroen
Copy link
Member

What

Experiment with the setComponent concept that is also used @ Payt.

☝️ @nickve28

Why

This will prove this model, and work as reference for other codebases. This can then also be referenced from the FE Golden Path documentation

Code Review

Please consider the following checklist when reviewing this Pull Request.
More background and details here.

  • Does the code actually solve the problem it was meant to solve?
  • Is the code covered by unit tests? Integration tests?
  • Does anything here need documentation? (Focus on why, not what.)
  • Does any of this code deal with privacy sensitive information or affects security? Ask an additional reviewer.
  • Is the code easy to understand and change in the future?
  • Is the same code or concept duplicated? Find a balance between DRYness and readability.
  • Does the code reasonably adhere to the Kabisa coding standards?
  • Be kind.

@nickve28
Copy link

Nice! Erg mooie uitvoer met TS typings ook. Wellicht zou wat JSdoc toevoegen ook helpen, dan heb je 2 vliegen in 1 klap (ook voor het golden path meteen)

@nickve28
Copy link

Je zou er nog voor kunnen kiezen om een 'wait for element to be removed' / loading indicator afwachten stap toe te voegen, dat heeft payt ook, ipv de standaard

const view = renderComponent()
await waitForElementTobeRemoved(() => expect(screen.queryByText("loading...")).not.toBeIntheDocument()
  setComponent(MyComponent, { waitForElementToBeRemoved: () => screen.queryByTestId('loading-icon') })

zij doen het zo, met een 'await' op renderComponent bijv

decorator: (Component: React.FC, settings: TSettings) => JSX.Element;
};

export const setComponent = <
Copy link

Choose a reason for hiding this comment

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

Function setComponent has a Cognitive Complexity of 11 (exceeds 5 allowed). Consider refactoring.

src/support/testing/testComponent.tsx Outdated Show resolved Hide resolved
src/support/testing/testComponent.tsx Outdated Show resolved Hide resolved
src/support/testing/testContexts.tsx Outdated Show resolved Hide resolved
src/support/testing/testContexts.tsx Outdated Show resolved Hide resolved
src/support/testing/testComponent.tsx Outdated Show resolved Hide resolved
src/support/testing/testDecorators.tsx Outdated Show resolved Hide resolved
),
};

export const dataDecorator = <TSerialized extends object>(
Copy link

Choose a reason for hiding this comment

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

Don't use object as a type. The object type is currently hard to use (see this issue).
Consider using Record<string, unknown> instead, as it allows you to more easily inspect and use the keys.

};

export const dataDecorator = <TSerialized extends object>(
mocks?: any,
Copy link

Choose a reason for hiding this comment

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

Argument 'mocks' should be typed with a non-any type.

src/support/testing/testDecorators.tsx Outdated Show resolved Hide resolved
* @param settings
* @returns
*/
export const setComponent = <
Copy link

Choose a reason for hiding this comment

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

Function setComponent has a Cognitive Complexity of 10 (exceeds 5 allowed). Consider refactoring.

src/support/testing/testDecorators.tsx Outdated Show resolved Hide resolved
export const tableDecorator: Decorator<"table", Record<string, unknown>> = {
name: "table",
settings: {},
decorator: (Component) => (
Copy link

Choose a reason for hiding this comment

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

Component definition is missing display name

*/
settings: TSettings;
/**
* Function do decorate incoming `Component`. This can be part of a larger chain.

Choose a reason for hiding this comment

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

'to decorate' denk ik

Copy link

@nickve28 nickve28 left a comment

Choose a reason for hiding this comment

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

lijkt mij prima zo

* @param settings
* @returns
*/
export const setTestSubject = <
Copy link

Choose a reason for hiding this comment

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

Function setTestSubject has a Cognitive Complexity of 15 (exceeds 5 allowed). Consider refactoring.

Copy link

codeclimate bot commented Jun 21, 2024

Code Climate has analyzed commit f72e56a and detected 7 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 1
Style 6

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

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

View more on Code Climate.

@matthijsgroen matthijsgroen marked this pull request as ready for review June 24, 2024 07:43
@matthijsgroen matthijsgroen merged commit 71ff371 into develop Jun 24, 2024
6 checks passed
@matthijsgroen matthijsgroen deleted the try-out-test-helper branch June 24, 2024 11:05
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.

5 participants