Add tests for table component #472

kr8n3r · 2018-01-29T11:25:52Z

We break this down into groups (as represented by describe blocks).

Trello https://trello.com/c/mdqq5RnP/645-automated-tests-for-table-component

NickColley

I think I prefer to be verbose and have lots of assertions rather than looping logic since it's hard to know what your intent was.

Not sure this is a blocker though.

36degrees · 2018-01-29T13:12:43Z

I think I would agree with Nick here. I find it really hard to follow the tests and to understand what the intent is. I think there's as much chance of introducing bugs in the logic of the tests as there is in having bugs in the code we're trying to test.

There's some good guidance on avoiding logic in tests here:

https://github.com/mawrkus/js-unit-testing-guide#avoid-logic-in-your-tests

kr8n3r · 2018-01-29T13:59:59Z

was hoping to cut down on the number of loops for the default example for each cell and row, when checking data.
how do you test if the data to render table is correct?

NickColley · 2018-01-29T14:35:30Z

@igloosi I suppose since we only have three rows it'd look like

loop 3
  assertion loop.item
endloop

assertion 'Item 1'
assertion 'Item 2'
assertion 'Item 3'

So the compromise is verbose tests, which for me personally is OK since looping behaviours are harder to grok and easier to hide bad assertions?

Hardcoding assertions feels dodgy since you'd want to have DRY code but in this case of testing it makes the intent of what you were asserting explicit.

kr8n3r · 2018-01-29T15:00:35Z

rewritten for more verbosity

alex-ju

Overall looks good to me. 👍

We have a lot of children 'search' that can be potentially reduced by using our BEM selectors (e.g. govuk-c-table__cell) to get a specific child element, otherwise being class-agnostic gives some flexibility, but I'm not sure we need it.

36degrees · 2018-01-31T09:40:16Z

src/components/table/template.test.js

+    const $componentBody = $component.children('tbody')
+    const $componentBodyRow = $componentBody.children('tr')
+
+    expect(parseInt($componentBodyRow.eq(0).children().eq(1).attr('colspan'), 10)).toBe(args.rows[0][1].colspan)


If we used toEqual here, could we get rid of the casting?

hmmm, with toEqual:

Expected value to equal:
2
Received:
"2"

kr8n3r · 2018-01-31T09:51:28Z

REVISED

NickColley · 2018-02-02T10:22:27Z

src/components/table/template.test.js

+      const { $ } = render('table', examples['table-with-caption-and-head'])
+      const $caption = $('.govuk-c-table__caption')
+
+      expect($caption).toHaveLength(1)


I don't think we need this assertion since the next one down will fail if the caption does not exist.

NickColley · 2018-02-02T10:25:26Z

src/components/table/template.test.js

+    })
+  })
+
+  describe('variant', () => {


Does this need to be in a describe block? What is it describing?

NickColley · 2018-02-02T10:29:23Z

src/components/table/template.test.js

+    const $tableRow = $component.find('.govuk-c-table .govuk-c-table__body .govuk-c-table__row')
+
+    expect($tableRow.eq(0).children().eq(0).get(0).tagName).toEqual('th')
+    expect($tableRow.eq(0).children().eq(0).hasClass('govuk-c-table__header')).toBeTruthy()


We could swap this around and make this a bit easier to read by doing

Instead of

expect($tableRow.eq(0).children().eq(0).get(0).tagName).toEqual('th') expect($tableRow.eq(0).children().eq(0).hasClass('govuk-c-table__header')).toBeTruthy()

try

const $tableBody = $component.find('.govuk-c-table .govuk-c-table__body') const $firstTableRow = $tableBody.find('.govuk-c-table__row:first-child') const $firstTableHeader = $firstTableRow.find('govuk-c-table__header:first-child') expect($firstTableHeader.get(0).tagName).toEqual('th') const $lastTableRow = $tableBody.find('.govuk-c-table__row:last-child') const $lastTableHeader = $lastTableRow.find('govuk-c-table__header:last-child') expect($lastTableHeader.get(0).tagName).toEqual('th')

alex-ju · 2018-02-02T10:37:29Z

src/components/table/template.test.js

+    })
+  })
+
+  describe('variant', () => {


could we get these two tests outside 'variant', even if they're using examples from yaml. also, and I know I'm being picky with this one, but can we order them somehow in a similar way we have them in the table or arguments (classes, caption..) would make it easier to follow and make sure we cover all the arguments.

NickColley

We need to replace

const { $ } = render()

with

const $ = render()

kr8n3r · 2018-02-04T20:18:34Z

@NickColley and @alex-ju update based on comments

alex-ju

Needs linting fix, otherwise looks neat 👍

alex-ju · 2018-02-05T09:24:01Z

src/components/table/template.test.js

+
+const examples = getExamples('table')
+describe('Table', () => {
+


Block must not be padded by blank lines

alex-ju · 2018-02-05T09:24:36Z

src/components/table/template.test.js

+    expect($firstTableHeader.get(0).tagName).toEqual('th')
+    expect($firstTableHeader.attr('scope')).toEqual('row')
+    expect($firstTableHeader.hasClass('govuk-c-table__header')).toBeTruthy()
+


More than one blank line not allowed

alex-ju · 2018-02-05T09:24:47Z

src/components/table/template.test.js

+    expect($lastTableHeader.get(0).tagName).toEqual('th')
+    expect($lastTableHeader.attr('scope')).toEqual('row')
+    expect($lastTableHeader.hasClass('govuk-c-table__header')).toBeTruthy()
+


Block must not be padded by blank lines

We break this down into groups (as represented by describe blocks). 1. Default table 2. Variants 3. Individual attributes

alex-ju

@NickColley, when you have a few minutes please have a look at the changes so we can unblock this one. thanks!

Refactoring based on my comment has been done.

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-472 January 29, 2018 11:26 Inactive

kr8n3r force-pushed the table-component-tests branch from 3a7132b to 7869b0e Compare January 29, 2018 11:28

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-472 January 29, 2018 11:28 Inactive

NickColley reviewed Jan 29, 2018

View reviewed changes

kr8n3r force-pushed the table-component-tests branch from 7869b0e to 69780ef Compare January 29, 2018 12:37

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-472 January 29, 2018 12:37 Inactive

kr8n3r force-pushed the table-component-tests branch from 69780ef to f657fb9 Compare January 29, 2018 14:04

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-472 January 29, 2018 14:05 Inactive

kr8n3r changed the title ~~WIP: Add Jest test for table component~~ WIP: Add tests for table component Jan 29, 2018

kr8n3r force-pushed the table-component-tests branch from f657fb9 to e4fa317 Compare January 29, 2018 14:09

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-472 January 29, 2018 14:09 Inactive

kr8n3r force-pushed the table-component-tests branch from e4fa317 to 48d365c Compare January 29, 2018 14:23

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-472 January 29, 2018 14:23 Inactive

kr8n3r force-pushed the table-component-tests branch from 48d365c to a73f3b6 Compare January 29, 2018 16:05

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-472 January 29, 2018 16:05 Inactive

kr8n3r force-pushed the table-component-tests branch from a73f3b6 to 87bd131 Compare January 29, 2018 16:08

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-472 January 29, 2018 16:08 Inactive

kr8n3r changed the title ~~WIP: Add tests for table component~~ Add tests for table component Jan 29, 2018

kr8n3r force-pushed the table-component-tests branch from 87bd131 to 3de46fa Compare January 30, 2018 08:59

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-472 January 30, 2018 08:59 Inactive

kr8n3r force-pushed the table-component-tests branch from 3de46fa to 0ce652a Compare January 30, 2018 09:01

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-472 January 30, 2018 09:02 Inactive

kr8n3r force-pushed the table-component-tests branch from 0ce652a to d8822f2 Compare January 30, 2018 16:37

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-472 January 30, 2018 16:37 Inactive

kr8n3r changed the title ~~Add tests for table component~~ Add tests for table component Jan 30, 2018

alex-ju reviewed Jan 31, 2018

View reviewed changes

36degrees reviewed Jan 31, 2018

View reviewed changes

kr8n3r force-pushed the table-component-tests branch from d8822f2 to 07dd601 Compare January 31, 2018 11:35

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-472 January 31, 2018 11:35 Inactive

kr8n3r force-pushed the table-component-tests branch from 07dd601 to d078cd2 Compare January 31, 2018 11:45

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-472 January 31, 2018 11:46 Inactive

kr8n3r force-pushed the table-component-tests branch from d078cd2 to c051427 Compare February 1, 2018 11:33

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-472 February 1, 2018 11:34 Inactive

NickColley reviewed Feb 2, 2018

View reviewed changes

alex-ju reviewed Feb 2, 2018

View reviewed changes

NickColley previously requested changes Feb 2, 2018

View reviewed changes

kr8n3r force-pushed the table-component-tests branch from c051427 to 0947e02 Compare February 2, 2018 15:30

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-472 February 2, 2018 15:30 Inactive

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-472 February 4, 2018 20:16 Inactive

kr8n3r force-pushed the table-component-tests branch from 42b051b to aaaea1c Compare February 4, 2018 20:17

govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-472 February 4, 2018 20:17 Inactive

alex-ju reviewed Feb 5, 2018

View reviewed changes

Jani Kraner added 3 commits February 5, 2018 10:25

Add Jest test for table component

46fd47d

We break this down into groups (as represented by describe blocks). 1. Default table 2. Variants 3. Individual attributes

Fix template classes attribute source

496fee5

Document table tests in changelog

e859d99

kr8n3r force-pushed the table-component-tests branch from aaaea1c to e859d99 Compare February 5, 2018 10:25

govuk-design-system-ci deployed to govuk-frontend-review-pr-472 February 5, 2018 10:25 View deployment

alex-ju approved these changes Feb 5, 2018

View reviewed changes

kr8n3r merged commit 3622ee1 into master Feb 5, 2018

kr8n3r deleted the table-component-tests branch February 5, 2018 12:53

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 table component #472

Add tests for table component #472

kr8n3r commented Jan 29, 2018 •

edited

Loading

NickColley left a comment

36degrees commented Jan 29, 2018 •

edited

Loading

kr8n3r commented Jan 29, 2018 •

edited

Loading

NickColley commented Jan 29, 2018 •

edited

Loading

kr8n3r commented Jan 29, 2018 •

edited

Loading

alex-ju left a comment •

edited

Loading

36degrees Jan 31, 2018

kr8n3r Jan 31, 2018

kr8n3r commented Jan 31, 2018 •

edited

Loading

NickColley Feb 2, 2018

NickColley Feb 2, 2018

kr8n3r Feb 4, 2018

NickColley Feb 2, 2018

alex-ju Feb 2, 2018

NickColley left a comment

kr8n3r commented Feb 4, 2018

alex-ju left a comment

alex-ju Feb 5, 2018

kr8n3r Feb 5, 2018

alex-ju Feb 5, 2018

kr8n3r Feb 5, 2018

alex-ju Feb 5, 2018

kr8n3r Feb 5, 2018

alex-ju left a comment •

edited

Loading


		const examples = getExamples('table')
		describe('Table', () => {

Add tests for table component #472

Add tests for table component #472

Conversation

kr8n3r commented Jan 29, 2018 • edited Loading

NickColley left a comment

Choose a reason for hiding this comment

36degrees commented Jan 29, 2018 • edited Loading

kr8n3r commented Jan 29, 2018 • edited Loading

NickColley commented Jan 29, 2018 • edited Loading

kr8n3r commented Jan 29, 2018 • edited Loading

alex-ju left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kr8n3r commented Jan 31, 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

NickColley left a comment

Choose a reason for hiding this comment

kr8n3r commented Feb 4, 2018

alex-ju left a 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

alex-ju left a comment • edited Loading

Choose a reason for hiding this comment

kr8n3r commented Jan 29, 2018 •

edited

Loading

36degrees commented Jan 29, 2018 •

edited

Loading

kr8n3r commented Jan 29, 2018 •

edited

Loading

NickColley commented Jan 29, 2018 •

edited

Loading

kr8n3r commented Jan 29, 2018 •

edited

Loading

alex-ju left a comment •

edited

Loading

kr8n3r commented Jan 31, 2018 •

edited

Loading

alex-ju left a comment •

edited

Loading