Add tests for input macro #478

alex-ju · 2018-01-29T15:55:15Z

This PR:

Adds tests to input component to ensure the markup is rendered correctly when providing arguments to the macro.
Updates CHANGELOG.MD.

Note to reviewers: if you feel like any example should be removed, let me know.

Trello Card

36degrees

Generally looking really good, though there's a couple of things that I think might want another look.

36degrees · 2018-01-29T15:58:44Z

src/components/input/template.test.js

+    it('renders with label', () => {
+      const { $ } = render('input', examples.default)
+      const $label = $('.govuk-c-label')
+      expect($label.text()).toContain('National Insurance number')


Should we have a test that the label is associated by id / for?

Oh, we do, but it's further up. What's the thinking being splitting these up into a separate context?

From line 97 I tested the examples that we have defined in the yaml to somehow make sure we're not breaking them. Before that line are 'by default' tests that cover the arguments, logic and relationships.

36degrees · 2018-01-29T16:04:12Z

src/components/input/template.test.js

+    it('renders with id', () => {
+      const { $ } = render('input', {
+        id: 'my-input',
+        name: 'my-input-name'


We don't test for name, and given that in the example above we omit both id and name, do we want to do the same here and just provide an id? Conversely, if we're providing name and id in the other examples because they are 'required', should we include them in the classes example also?

Indeed I started to add id and name because they are 'required', but I'm not sure it's the right approach and right now is inconsistent across examples. I would rather add the minimum number of attributes since we cannot really enforce the 'required' state.

Removed unused attributes for now, even if defined as 'required'.

36degrees · 2018-01-30T09:02:44Z

src/components/input/template.test.js

+      expect($label.attr('for')).toEqual('my-input')
+    })
+
+    it('renders with error message', () => {


Would it make sense to split this test into two parts?

it applies the error variant class if an error message is passed

it renders an error message if passed

36degrees · 2018-01-30T09:03:17Z

src/components/input/template.test.js

+
+    it('renders with label', () => {
+      const { $ } = render('input', {
+        id: 'my-input',


This is redundant

removed the other one that was testing the example and the label too

36degrees · 2018-01-30T09:04:29Z

src/components/input/template.test.js

+    it('renders with attributes', () => {
+      const { $ } = render('input', {
+        attributes: {
+          'aria-describedby': 'my-label'


It might be worth using a different 'non-sensical' data attribute, only to avoid confusion for people scanning through who might mis-read this as inputs actually having or requiring some aria markup?

replaced aria-describedby with 'non-sensical' data attribute

36degrees · 2018-01-30T09:05:21Z

src/components/input/template.test.js

+
+      const $label = $('.govuk-c-label')
+      expect($label).toBeTruthy()
+      expect($label.attr('for')).toEqual('my-input')


Again, suggest this is split into two parts - one that tests that the label is rendered correctly (probably worth checking the text itself rather than just ensuring the element exists?) and a second test that ensures the label is correctly associated with the input?

I expect the label to be tested within the label macro. We don't add any logic about the label in the input macro, except for setting label's for attribute equal to the input's id.

OK. I still think it's worth having an explicit test for that for/id relationship.

36degrees · 2018-01-30T09:05:52Z

src/components/input/template.test.js

+    it('renders with hint text', () => {
+      const { $ } = render('input', examples['with-hint-text'])
+      const $hintText = $('.govuk-c-label__hint')
+      expect($hintText.text().trim()).toEqual('It’s on your National Insurance card, benefit letter, payslip or P60. For example, ‘QQ 12 34 56 C’.')


Would be good to wrap this within 80 chars.

hannalaakso

Looks good to me. Thanks for talking me through the rationale of how we're grouping tests @alex-ju (I'll write these up for our docs).

I don't know if you want to have another look at this PR @36degrees before it gets merged?

NickColley · 2018-02-02T11:37:38Z

src/components/input/template.test.js

+      })
+
+      const $errorMessage = $('.govuk-c-error-message')
+      expect($errorMessage).toBeTruthy()


Could we replace these truthy tests with snapshots?

alex-ju · 2018-02-02T11:57:49Z

Updated with snapshots as discussed.

NickColley · 2018-02-02T15:31:31Z

src/components/input/template.test.js

+        }
+      })
+
+      expect(htmlWithClassName($, '.govuk-c-label')).toMatchSnapshot()


Could we wrap these snapshots tests with a description 'dependant components' or similar?

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-478 January 29, 2018 15:55 Inactive

alex-ju force-pushed the add-tests-for-input-macro branch from d311bf5 to c13f2fb Compare January 29, 2018 15:55

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-478 January 29, 2018 15:56 Inactive

36degrees reviewed Jan 29, 2018

View reviewed changes

alex-ju force-pushed the add-tests-for-input-macro branch from c13f2fb to b4607fd Compare January 29, 2018 17:47

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-478 January 29, 2018 17:47 Inactive

alex-ju force-pushed the add-tests-for-input-macro branch from b4607fd to ac6707c Compare January 29, 2018 17:51

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-478 January 29, 2018 17:51 Inactive

36degrees reviewed Jan 30, 2018

View reviewed changes

alex-ju force-pushed the add-tests-for-input-macro branch from ac6707c to 1284fd9 Compare January 30, 2018 10:22

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-478 January 30, 2018 10:22 Inactive

alex-ju force-pushed the add-tests-for-input-macro branch from 1284fd9 to 4d5edfb Compare January 30, 2018 10:27

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-478 January 30, 2018 10:27 Inactive

alex-ju force-pushed the add-tests-for-input-macro branch from 4d5edfb to 84a9c5c Compare January 30, 2018 10:32

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-478 January 30, 2018 10:32 Inactive

hannalaakso approved these changes Feb 1, 2018

View reviewed changes

alex-ju force-pushed the add-tests-for-input-macro branch from 84a9c5c to 04370bf Compare February 2, 2018 11:01

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-478 February 2, 2018 11:01 Inactive

NickColley reviewed Feb 2, 2018

View reviewed changes

alex-ju force-pushed the add-tests-for-input-macro branch from 04370bf to f5773be Compare February 2, 2018 11:54

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-478 February 2, 2018 11:54 Inactive

NickColley reviewed Feb 2, 2018

View reviewed changes

alex-ju force-pushed the add-tests-for-input-macro branch from f5773be to 826947d Compare February 2, 2018 15:37

govuk-design-system-ci deployed to govuk-frontend-review-pr-478 February 2, 2018 15:37 View deployment

alex-ju added 2 commits February 2, 2018 15:44

Add tests for input macro

49fd6fd

Update changelog

826947d

NickColley approved these changes Feb 2, 2018

View reviewed changes

alex-ju merged commit c787e9a into master Feb 2, 2018

alex-ju deleted the add-tests-for-input-macro branch February 2, 2018 16:27

alex-ju mentioned this pull request Feb 20, 2018

Frontend release 0.0.23 #537

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests for input macro #478

Add tests for input macro #478

alex-ju commented Jan 29, 2018 •

edited by hannalaakso

Loading

36degrees left a comment

36degrees Jan 29, 2018

36degrees Jan 29, 2018

alex-ju Jan 29, 2018 •

edited

Loading

36degrees Jan 29, 2018

alex-ju Jan 29, 2018

alex-ju Jan 29, 2018

36degrees Jan 30, 2018

alex-ju Jan 30, 2018

36degrees Jan 30, 2018

alex-ju Jan 30, 2018 •

edited

Loading

36degrees Jan 30, 2018

alex-ju Jan 30, 2018

36degrees Jan 30, 2018

alex-ju Jan 30, 2018

36degrees Jan 30, 2018

alex-ju Jan 30, 2018

36degrees Jan 30, 2018

hannalaakso left a comment •

edited

Loading

NickColley Feb 2, 2018

alex-ju commented Feb 2, 2018

NickColley Feb 2, 2018

Add tests for input macro #478

Add tests for input macro #478

Conversation

alex-ju commented Jan 29, 2018 • edited by hannalaakso Loading

36degrees left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-ju Jan 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-ju Jan 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hannalaakso left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-ju commented Feb 2, 2018

Choose a reason for hiding this comment

alex-ju commented Jan 29, 2018 •

edited by hannalaakso

Loading

alex-ju Jan 29, 2018 •

edited

Loading

alex-ju Jan 30, 2018 •

edited

Loading

hannalaakso left a comment •

edited

Loading