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

refactor(refs T29004): re-add label tests #591

Merged
merged 3 commits into from
Oct 20, 2023
Merged

Conversation

hwiem
Copy link
Contributor

@hwiem hwiem commented Oct 18, 2023

Ticket: https://yaits.demos-deutschland.de/T29004

  • the label tests (that were disabled after refactoring the label props to be a single object) are fixed and enabled again
  • some more tests that were disabled in DpInput.spec.js for unknown reasons are enabled again as well

How to test

Run the tests, there should be no error

- the label tests (that were disabled after
refactoring the label props to be a single
object) are fixed and enabled again
- some more tests that were disabled in
DpInput.spec.js for unknown reasons are
enabled again as well
@hwiem hwiem requested a review from salisdemos October 18, 2023 14:50
@hwiem hwiem self-assigned this Oct 18, 2023
Copy link
Contributor

@salisdemos salisdemos left a comment

Choose a reason for hiding this comment

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

Thank You!

Copy link
Contributor

@spiess-demos spiess-demos left a comment

Choose a reason for hiding this comment

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

Why do we have to run the label tests in all other component tests, too? isn't it sufficient to test the label component for itself?

@hwiem
Copy link
Contributor Author

hwiem commented Oct 19, 2023

Why do we have to run the label tests in all other component tests, too? isn't it sufficient to test the label component for itself?

That's a very good question. By running them in other components we ensure that the props are passed properly from the outer component to DpLabel, but maybe that's not really the purpose of unit tests?

@spiess-demos
Copy link
Contributor

By running them in other components we ensure that the props are passed properly from the outer component to DpLabel, but maybe that's not really the purpose of unit tests?

As runLabelTests sets the props by itself, this is not exactly true. Testing child components in a component test looks (at least a little bit) like testing the implementation. To ensure that each form component has a working label implementation we could change runLabelTests so that it takes the props in question (text, hint, required and so on) as a second param, and then expexts the rendered html to contain those values within their expected html nodes.

@hwiem
Copy link
Contributor Author

hwiem commented Oct 20, 2023

By running them in other components we ensure that the props are passed properly from the outer component to DpLabel, but maybe that's not really the purpose of unit tests?

As runLabelTests sets the props by itself, this is not exactly true. Testing child components in a component test looks (at least a little bit) like testing the implementation. To ensure that each form component has a working label implementation we could change runLabelTests so that it takes the props in question (text, hint, required and so on) as a second param, and then expexts the rendered html to contain those values within their expected html nodes.

I'm not sure I understand your concern. Why is it better to pass the props as a parameter?

@spiess-demos
Copy link
Contributor

I'm not sure I understand your concern. Why is it better to pass the props as a parameter?

Thanks for clarifying the runLabelTests approach in an 1:1. I simply did not look close enough, the test already does what i thought it should do.

Copy link
Contributor

@spiess-demos spiess-demos left a comment

Choose a reason for hiding this comment

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

lgtm

@hwiem hwiem merged commit bfd5c1e into main Oct 20, 2023
3 checks passed
@hwiem hwiem deleted the f_T29004_re_add_label_tests branch October 20, 2023 13:29
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.

3 participants