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

Use non-bool type strings for attributes in test fixtures #2071

Merged
merged 1 commit into from
Dec 16, 2020
Merged

Use non-bool type strings for attributes in test fixtures #2071

merged 1 commit into from
Dec 16, 2020

Conversation

matthew-shaw
Copy link
Contributor

@matthew-shaw matthew-shaw commented Dec 15, 2020

Follow on from #2064:

When using boolean cast-able string values in test fixtures, these can be interpreted as actual boolean types by ports of govuk-frontend, such as the govuk-frontend-jinja Python port. For example, three tests fail due to casing differences between the expected string values of "true" and "false" and their boolean equivalents True and False: https://github.com/LandRegistry/govuk-frontend-jinja/pull/10/checks?check_run_id=1538392387#step:6:1365

Other options:

  • I could code around this in my port by type-checking all param values and lower casing only bool cast-able values as strings. However, it seems better to improve the upstream test fixtures to use more explicit string type values instead.
  • I could blanket lower case all attribute values, but this could break things with unexpected behaviour and would deviate from how the upstream original behaves.

Changing these test fixture params to non-bool type strings will make maintaining ports of govuk-frontend in other frameworks and languages easier.

@matthew-shaw
Copy link
Contributor Author

Ping @36degrees 👍

Copy link
Contributor

@36degrees 36degrees left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for contributing and for your patience, @matthew-shaw 🙏🏻

This just needs another review from someone else on the team and then we can get this merged for you.

@36degrees 36degrees changed the title Use non-bool type strings in test fixtures Use non-bool type strings for attributes in test fixtures Dec 15, 2020
@36degrees 36degrees merged commit 04097d8 into alphagov:master Dec 16, 2020
@matthew-shaw matthew-shaw deleted the test-fixture-bools branch December 16, 2020 15:14
@36degrees 36degrees mentioned this pull request Dec 17, 2020
@matthew-shaw matthew-shaw restored the test-fixture-bools branch December 17, 2020 15:35
@matthew-shaw
Copy link
Contributor Author

@36degrees Unfortunately when I recreated this PR from the original I also missed one of the changes required. There's a fixture with the same issue for the details component. I've created a new PR (#2079) to resolve that one. So sorry about all the hassle! 😞

@36degrees
Copy link
Contributor

No worries. Unfortunately we've already shipped the 3.10.2 release with the previous changes in it (we haven't announced it yet as we're having some issues getting the docs updated), and we're unlikely to be doing another release for a few weeks now.

@matthew-shaw
Copy link
Contributor Author

That's ok, I'll wait for the next fix release before doing a new release of the Jinja port. Cheers 🎄

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.

3 participants