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

Add tests for govuk-c-radios component #476

Merged
merged 5 commits into from
Feb 1, 2018

Conversation

NickColley
Copy link
Contributor

@NickColley NickColley commented Jan 29, 2018

  • includes new example disabled radios
  • includes new example with all fieldset params set

Uses snapshot testing to test the dependant components - Label and Fieldset.
This ensures that all values that can be passed through the Radio components params through to
either component is tested, and if we change the behavour in either the Label or Fieldset component
we will see a change to confirm in this component.

Snapshot testing makes use of a custom serializer to make the diffs easy to read

As part of https://trello.com/c/G8rFbXWB/641-automated-tests-for-radios-component

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-476 January 29, 2018 14:38 Inactive
expect($component.find('.govuk-c-error-message').text()).toContain('Please select an option')

const $fieldset = $('.govuk-c-fieldset')
expect($fieldset.hasClass('app-c-fieldset--custom-modifier')).toBeTruthy()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lots of assertions here since we just want to know that the fieldset plays nice inside this component. Feels over the top to do individual test blocks (which should be done in the fieldset component tests), maybe a snapshot test would be better here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See the same for label and error message.

@NickColley NickColley force-pushed the add-tests-for-radios-component branch from 0c387c0 to 960238a Compare January 29, 2018 15:25
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-476 January 29, 2018 15:25 Inactive
@NickColley NickColley force-pushed the add-tests-for-radios-component branch from 960238a to d14bafb Compare January 29, 2018 15:29
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-476 January 29, 2018 15:29 Inactive
@NickColley NickColley force-pushed the add-tests-for-radios-component branch from d14bafb to 220246f Compare January 29, 2018 15:30
@NickColley NickColley changed the title WIP: Add tests for radios component Add tests for govuk-c-radios component Jan 29, 2018
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-476 January 29, 2018 15:30 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-476 January 29, 2018 15:31 Inactive
const $firstInput = $component.find('.govuk-c-radios__item:first-child input')
const $firstLabel = $component.find('.govuk-c-radios__item:first-child label')
expect($firstInput.attr('name')).toEqual('example-name')
expect($firstLabel.text()).toEqual('example-name')
Copy link
Contributor

Choose a reason for hiding this comment

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

Fails here. I think you meant:
expect($firstLabel.text().trim()).toEqual('Yes')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Might be able to use toContain, will update

const $lastInput = $component.find('.govuk-c-radios__item:last-child input')
const $lastLabel = $component.find('.govuk-c-radios__item:last-child label')
expect($lastInput.attr('name')).toEqual('example-name')
expect($lastLabel.text()).toEqual('example-name')
Copy link
Contributor

Choose a reason for hiding this comment

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

expect($lastLabel.text().trim()).toEqual('No')

@NickColley NickColley force-pushed the add-tests-for-radios-component branch from 6324659 to 7b48994 Compare January 30, 2018 10:22
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-476 January 30, 2018 10:22 Inactive
@NickColley NickColley force-pushed the add-tests-for-radios-component branch from 7b48994 to 391a18d Compare January 30, 2018 10:28
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-476 January 30, 2018 10:28 Inactive
Copy link
Contributor

@alex-ju alex-ju left a comment

Choose a reason for hiding this comment

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

Overall looks good to me, but if we're aiming for single expects per test should probably need a lot of splitting.

Update: had a chat with Nick and decided to use snapshots for interconnected components.

alex-ju
alex-ju previously approved these changes Jan 31, 2018
@NickColley NickColley force-pushed the add-tests-for-radios-component branch from 391a18d to 56acbbb Compare January 31, 2018 17:58
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-476 January 31, 2018 17:58 Inactive
@NickColley NickColley changed the title Add tests for govuk-c-radios component WIP: Add tests for govuk-c-radios component Feb 1, 2018
@NickColley
Copy link
Contributor Author

Just setting this to WIP, need to update based on Alex's feedback

@NickColley NickColley force-pushed the add-tests-for-radios-component branch from 56acbbb to da536da Compare February 1, 2018 11:38
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-476 February 1, 2018 11:38 Inactive
@NickColley NickColley changed the title WIP: Add tests for govuk-c-radios component Add tests for govuk-c-radios component Feb 1, 2018
NickColley and others added 3 commits February 1, 2018 11:40
Includes new example disabled radios
Uses shapshots tests against label, fieldset components.
Adds an extreme fieldset example to test all params that can be passed through.
@NickColley NickColley force-pushed the add-tests-for-radios-component branch from da536da to 35d2c99 Compare February 1, 2018 11:42
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-476 February 1, 2018 11:42 Inactive
@NickColley
Copy link
Contributor Author

Updated, I've added a new jest helper method that will allow us to only snapshot parts of the HTML we care about.

Would appreciate your thoughts, does it make sense?

@NickColley NickColley dismissed alex-ju’s stale review February 1, 2018 12:04

New changes have been made

@NickColley
Copy link
Contributor Author

Talked to Alex, going to add some comments to document the jest-helpers

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-476 February 1, 2018 12:49 Inactive
@NickColley NickColley force-pushed the add-tests-for-radios-component branch from ec1f522 to 0b1b11f Compare February 1, 2018 12:51
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-476 February 1, 2018 12:51 Inactive
* @param {string} params parameters that are used in the component template
* @returns {object} returns object that includes raw HTML (output) and
* also a cheerio (jQuery) instance of the template for easy DOM querying
*/
function render (componentName, params) {
const output = nunjucks.render(componentName + '/template.njk', { params })
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wonder if we need this output anymore since it was originally used for snapshot testing...

If we're using the htmlWithClassName function, the output
returned value from render is now not useful.
Copy link
Contributor

@alex-ju alex-ju left a comment

Choose a reason for hiding this comment

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

Thanks for the updates @NickColley!
I think it's worth saying that the snapshots are ensuring us that if the component changes, the way it's rendered within or in connection with other components doesn't change.

I can approve this again, but would be good to have a second pair of eyes on this, maybe from @hannalaakso too as she will be working o the govuk-c-checkboxes in a similar way.

Update: talked to Hanna about it.

@NickColley NickColley merged commit 2b8b4da into master Feb 1, 2018
@NickColley NickColley deleted the add-tests-for-radios-component branch February 1, 2018 17:53
@NickColley
Copy link
Contributor Author

Will write up this approach for the docs

@alex-ju
Copy link
Contributor

alex-ju commented Feb 1, 2018

@NickColley, @hannalaakso also wants to update the docs on how to write tests, would be good to catch up or split the work

@hannalaakso
Copy link
Member

Sounds good to me. @NickColley go ahead and write up your approach and I can contribute to the docs 🙂

@alex-ju alex-ju mentioned this pull request Feb 20, 2018
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.

4 participants