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

Provide a way for maintainers of ports to check that they're using equivalent HTML markup #1830

Closed
36degrees opened this issue Jun 9, 2020 · 24 comments
Labels
epic Epics are used in planning project boards to group related stories
Milestone

Comments

@36degrees
Copy link
Contributor

What

Provide a way for maintainers of ports to ensure that, for given scenarios, their port generates markup equivalent to that of GOV.UK Frontend.

Why

Whilst anyone can easily re-use the stylesheets and JavaScript from GOV.UK Frontend, most of our users have to maintain their own abstractions of the HTML markup used for a component.

It's important for that markup to be equivalent to the markup used in GOV.UK Frontend so that it renders the same and has the same accessibility. This is especially true when those abstractions are published as packages to be re-used by others.

Further detail

Possible implementations might include:

  • example ‘fixtures’ that provide the expected HTML output for a given set of inputs, optionally with a test suite that makes assertions using those fixtures and some sort of test harness implemented by the maintainer – a little like snapshot testing
  • a test suite that makes assertions about the HTML for a given input, for example asserting that elements have specific attributes or relationships

Questions:

  • How would fixtures relate to the component YAML files?
  • Do these supplement or replace any of our existing tests? (Related: If using fixtures, do we generate them as part of the release process or as we make changes?)
  • Do we release tests as a separate package or as part of the existing GOV.UK Frontend package?
  • How thoroughly do we test components that are included by other components?
  • What about less widely supported features, like {% call %}?
  • How do we support ports that have deviated from the API (e.g. if they chose to implement sanitisation differently)
  • How do we ensure coverage?
  • What about scenarios where class names are changed (e.g. using local identifiers in React)
@36degrees 36degrees added the epic Epics are used in planning project boards to group related stories label Jun 9, 2020
@matthew-shaw
Copy link
Contributor

This would be really useful for a Jinja port I've recently started: https://github.com/LandRegistry/govuk-frontend-jinja

The code in the repo above is generated by an automated script that creates Jinja equivalent macros for each of the Nunjucks ones. This is currently run within each frontend app itself at build time. I'd rather this was an installable package from PyPi to help manage version upgrades, remove some additional complexity from the app, and have a robust set of tests to prove compatibility. Ideally, I want to render the HTML output using a single set of inputs, for both the original Nunjucks and Jinja ported macros, then diff and fail tests if they're not identical.

As you can see, not got very far with this yet so feedback/contributions very welcome!

@andymantell
Copy link
Contributor

andymantell commented Jun 10, 2020

In govuk-react-jsx I render the nunjucks templates using the yaml example data in each component, and then do the same for the react components. I then use https://github.com/Prettyhtml/prettyhtml to smooth over minor rendering differences and https://github.com/markedjs/html-differ to diff the results. It's not the prettiest code but you can see it here:

https://github.com/surevine/govuk-react-jsx/blob/master/tests/utils/govuk-frontend-diff.js

which is then called from each component's tests:

https://github.com/surevine/govuk-react-jsx/blob/master/src/govuk/components/accordion/accordion.test.js

I then get test output like this (in this case, I have removed the role="banner" from the header to demonstrate)

image

This works well for the most part. The only issue I have encountered is cases where the example yaml data doesn't fully exercise the component. There's also a few superficial differences in the output but I managed to configure the html differ to ignore these.

Having a set of pre-rendered fixtures would certainly simplify my test suite and I could concentrate on just the diffing. Or indeed a test suite where I pass in my html and everything is handled for me.

I might have a go at abstracting out the work I did on govuk-react-jsx and see if it can help @matthew-shaw out above - as a proof of concept almost.

@36degrees
Copy link
Contributor Author

@matthew-shaw have you seen https://github.com/alphagov/govuk-frontend-jinja? (maintained by a separate team at GDS, not the Design System team)

@andymantell I'm not sure you tagged the right person there!

We think the first step will be to create a proposal that we can share with the community – once we've done that we'll make sure you get the chance to feed back. These comments will be really helpful in shaping that proposal, so thank you both!

@andymantell
Copy link
Contributor

andymantell commented Jun 10, 2020

@36degrees The govuk-frontend-jinja repo at https://github.com/alphagov/govuk-frontend-jinja repository uses a port of a script I wrote back at HM Land Registry to automatically port the Nunjucks across to Jinja using some regular expressions and other tomfoolery. I believe they've taken it a bit further than I had originally and fixed some issues I hadn't managed to. But it's similar conceptually.

Before I left HMLR though I was growing increasingly frustrated with the automated porting script and had basically concluded that it wasn't a great way to do things - there are so many subtle differences between Nunjucks and Jinja that the script became harder to maintain than just manually porting between the two! @matthew-shaw and I have chatted about this at length and he is working on the successor to this which is a set of manual ports supported by a test suite exactly as you described in this ticket. (i.e. the same model I use for govuk-react-jsx)

I think if I have a crack at packaging up my test suite it might be an interesting exercise to see how easily it can be applied to @matthew-shaw's work

Not proposing you use my code particularly, but it might shed some light on any potential issues with such a solution.

@lfdebrux
Copy link
Member

lfdebrux commented Jun 10, 2020

@andymantell interesting, we actually found that Nunjucks and Jinja templates are similar enough that we haven't really had to worry about maintaining https://github.com/alphagov/govuk-frontend-jinja (which of course we couldn't have done without you, so thanks again!).

That said, that could be mainly because we haven't been closely following the master branch of govuk-frontend, but we've also been helped by a comprehensive test suite. Much as you and @matthew-shaw described, it's based on the YAML examples; except we copied and pasted them into our codebase :/

@lfdebrux
Copy link
Member

Also, I think that @penx who created the issue #1095 already had a way of extracting and packaging the govuk-frontend exampes

https://github.com/penx/govuk-frontend-template-spec

@andymantell
Copy link
Contributor

Oh nice. govuk-frontend-template-spec looks interesting, I'll take a look at it.

@lfdebrux Regarding the Jinja porting script - I found myself adding more and more edge cases to the conversion as each govuk-frontend release came out and it just didn't feel like an clean and enjoyable way to achieve it. I manually replicate the govuk-frontend changes into govuk-react-jsx and it's really easy just to step through the changelog when releases happen and implement them again in JSX (backed up by the tests). Doing it this way has felt much tidier (to me at least).

I reckon @matthew-shaw should pursue this solution (and I'll pitch in and help where I can) and maybe we could all have a chat at some point at compare where we've ended up. It would be good to get one published to PyPi.

I've probably derailed this thread massively. Sorry! :-)

@DanScottNI
Copy link

We (Department of Finance Northern Ireland) have a c# port in development of the nunjucks macros.

A way to test that the output is largely the same as the nunjucks macros would be very useful.

Definitely going to look into govuk-frontend-template-spec.

@gunndabad
Copy link

@DanScottNI is your C# port open source?

@andymantell
Copy link
Contributor

I've been working on extracting the tests from govuk-react-jsx into their own cli tool this week. It's here at https://github.com/surevine/govuk-frontend-diff if anyone wants to give it a whirl.

It expects you to implement a tiny http server responding to /component/<component-name> and /template routes. You run the tool against this server and it passes the example data and expects html responses in return. It then diffs them against the reference Nunjucks implementation and passes or fails accordingly.

There's a pull request open at LandRegistry/govuk-frontend-jinja#6 for a demo of how to integrate with it. Happy to help out with others if repositories are open source.

@matthew-shaw
Copy link
Contributor

matthew-shaw commented Jun 26, 2020

As @andymantell says, the govuk-frontend-jinja package is now published on PyPi. I've also deployed a little demo Flask app to Heroku with a full component specimen of every example from the GOV.UK Design System. This is now also being used internally within HM Land Registry reference/template implementations we call "skeleton apps" and I'm working on getting our service teams to adopt it.

This has already been a very worthwhile exercise. Using Andy's test suite, we now have 190 tests for every single component in every single permutation from the design system. In doing that, we've found some bugs with our original port that have now been fixed, so we already have a provably higher quality output than before. This means we can be certain of 100% markup parity and compliance with the latest, and every subsequent, GOV.UK Frontend release. This has also enabled me to remove a bunch of complexity from our skeleton frontend application, and simplify that into an open source component that the cross government community can make use of too.

The published package is currently at release 0.2.1 and I intend to leave it in a beta stage until at least the next release of GOV.UK Frontend. Once I can prove that doing an update to the latest GOV.UK release is quick and accurate I'll bump it to version 1.0.0 and mark it as production ready. In all likelihood very little will change, and having that comprehensive test suite will ensure its quality.

@penx
Copy link

penx commented Jun 26, 2020

Hi all, I made govuk-frontend-template-spec, just to let you know I've subbed to this thread and happy to help where I can. I'm not working on govuk projects at the moment, but If anyone wants to collaborate on it to make it a decent stop gap solution until there's an official solution then please ping me.

@DanScottNI
Copy link

@DanScottNI is your C# port open source?

Not yet. Still very early stages of development, and we're still in the early stages of open sourcing things.

@gunndabad
Copy link

@DanScottNI is your C# port open source?

Not yet. Still very early stages of development, and we're still in the early stages of open sourcing things.

I have an ASP.NET Core Tag Helper port of most of these components at https://github.com/gunndabad/govuk-frontend-aspnetcore that may be of interest. Still early days so (very) light on docs.

@vanitabarrett
Copy link
Contributor

We're going to start including fixture files in the published GOV.UK Frontend package.

If you maintain your own macros, templates or partials you can use the fixtures to check that your components output the same HTML that GOV.UK Frontend uses, for example:

  • when you first create the macro, template or partial for a component
  • whenever you update to a newer version of GOV.UK Frontend

We'd like your feedback on this proposed change. You can:

Please provide feedback by 5pm on Tuesday 4 August.

Problem

Whilst you can easily re-use the stylesheets and JavaScript from GOV.UK Frontend, it's likely that you have to maintain the HTML markup used for each component yourself.

At the minute there's no automated way for you to tell whether your HTML is the same as the HTML that GOV.UK Frontend uses.

You may already be using the examples in the component YAML files as test fixtures, but:

  • you have to manually fetch the YAML files from the src directory in GitHub, because they're not included in the published package
  • the examples in the YAML file are not comprehensive, and do not cover all of the ways the component can be used
  • you have to include Nunjucks somewhere in your development or test environment to generate the expected HTML using the GOV.UK Frontend macros

Proposal

We'll make the examples provided for each component more comprehensive, so that every component option is represented by at least one example.

We'll then publish the examples in a fixtures.json file for each component, which will contain an array of objects representing each example with the following properties:

  • name
  • options – the options passed to the component
  • html – the HTML as rendered by the component's Nunjucks macro

For example, in the published package you'd find govuk/components/button/fixtures.json with examples that look like this:

[
  {
    "name": "default",
    "options": {
      "text": "Save and continue"
    },
    "html": "<button class=\"govuk-button\" data-module=\"govuk-button\">\n  Save and continue\n</button>"
  },
  // […]
]

You can use these fixture files in your own tests to check that your HTML is the same as the HTML that GOV.UK Frontend uses.

For example, in pseudo-code:

fixtures = load_json_file("govuk/components/button/fixtures.json")
for fixture in fixtures
	expected_ouput = fixture.html
	actual_output = render_my_button(fixture.options)
	expect_equal(actual_output, expected_output)
end

You may need to normalise the expected and actual HTML to allow for differences in whitespace or indentation.

You can try a pre-release of GOV.UK Frontend that includes a fixtures.json file for the button component only by running:

npm install alphagov/govuk-frontend#634eeca76ee6f4399cba54cc13a11bd38f5c450a

@andymantell
Copy link
Contributor

@vanitabarrett Are the current YAML files going to remain, or do the new fixtures.json files replace them?

Reason I ask is that I use the examples in the YAML files to build the storybook demos on govuk-react-jsx. If they're disappearing I'll do something else instead... (I.e. I'm not saying you have to keep them, just thinking ahead).

As for the fixtures themselves, it sounds perfect.

@penx
Copy link

penx commented Jul 28, 2020

@36degrees @vanitabarrett having a read through, that sounds spot on. I'll try do a React PoC at some point.

Some things to think about:

  • how should we (govuk-frontend authors and users) deal with html options? For some frameworks, such as React, you wouldn't pass html as a property (you would use the children prop containing React components rather than raw html), so it may be hard to adhere to a fixture that includes an html option. Perhaps frameworks would skip these fixtures? but if that's the case it would be good to ensure that fixtures with a text option were always available as an alternative when demonstrating a feature unrelated to the html option.
  • please can you confirm that the fixtures.json will be part of the npm release? I assume this is the case, but I know the last time I looked, the tests are on github but not in the npm package
  • would it be possible to add a fixtures index somewhere? This would mean that if you have a component library that intends to have 100% coverage, and a new component is added, then this could get picked up by tests. In fact, as a library author, you may be able to point to this file and then automate generating/running the rest of the tests.

e.g.
fixtures.json

{
  "button": "./govuk/components/button/fixtures.json",
  "etc.": "...etc"
}

@vanitabarrett
Copy link
Contributor

Thank you for the comments @penx @andymantell

Some parts of the proposal were perhaps not as clear as they could be:

  • fixtures.json files will be included in the npm release. All fixture files will be stored at a common path: /package/govuk/components/[component-name]/fixtures.json. We expect maintainers to iterate through these to ensure 100% coverage
  • YAML files in /src will remain, but we recommend replacing your use of them (where possible) with the new fixtures. We treat /src as internal to the team, so files like the component yaml could be changed in future and this wouldn’t normally be treated as a breaking change.

@penx Our aim is to make these fixtures as generic as possible. In the same way that maintainers may need to normalise the HTML to allow for differences, we expect any differences in framework behaviour to be handled by maintainers. If there’s anything we can do to make the process smoother, let us know!

The examples in GOVUK Frontend should only pass html where we specifically want to check the HTML works correctly. If you want to skip these, you could skip any tests that pass html to a component.

@penx
Copy link

penx commented Jul 29, 2020

The examples in GOVUK Frontend should only pass html where we specifically want to check the HTML works correctly

👌 if this is consistently followed then this works for me, thanks!

peteryates added a commit to x-govuk/govuk-form-builder that referenced this issue Jul 29, 2020
This is an initial attempt at using GOV.UK Frontend's recent fixtures[0]
addition to ensure the HTML we're generating is in accordance with the
Design System.

After flipping a few default values it's not a million miles off, but
there are some snags:

1) Rails insists on adding a `data-disable-with` attribute to the
   submit[1], even when we attempt to disable it by passing `false`

2) Because in all cases (other than `#govuk_date_field`) we are merely
   wrapping the built-in Rails form helpers, we're not in control of the
   order of attributes. Comparing them directly will fail.

Both of these problems are demonstrated in the RSpec output:

  expected: `<input value="Start now" type="submit" name="start-now" class="govuk-button" data-module="govuk-button">`
       got: `<input type="submit" name="start-now" value="Start now" class="govuk-button" data-module="govuk-button" data-disable-with="Start now" />`

[0] alphagov/govuk-frontend#1830
[1] https://api.rubyonrails.org/classes/ActionView/Helpers/FormTagHelper.html#method-i-submit_tag
@frankieroberto
Copy link
Contributor

frankieroberto commented Jul 29, 2020

I like this idea, and will experiment with the button example in our port.

A few questions:

  • Can this fixture be included for the components which aren't listed in the Design System website, but which are used by other components (eg govukFieldset and govukLabel)?
  • Any thoughts on how we could specify and test the equivalent of the caller option, which some of the components accept (and which govukFieldset kinda expects).
  • Can the JSON files be published at a standard location (tagged with the version), so that ports don’t have to use npm to require them? (As lots of languages have their own package manager, and using two package managers alongside each other can be a pain). I can see some ports wanting to commit the fixtures into their repo, and just use a script to update it from a known location when supporting a higher govuk-frontend version.
  • How would this file be maintained? I'm presuming that the file would be auto-generated from some other source, such as the existing YAML files?

Minor suggestion:

  • Make the top-level of the JSON file be an object, rather than an array, and move all the examples under an "examples" key. Mainly to allow additional keys / data to be added in the future without breaking backwards-compatibility.

@vanitabarrett
Copy link
Contributor

Thanks for your comment @frankieroberto !

We will generate fixtures for all components in GOVUK Frontend, so the fieldset and label will have fixtures. The fixtures are generated from the YAML files, so anything with a YAML file will get a corresponding fixtures file.

Any components that support caller should also have another option, for example: passing HTML instead. We would suggest taking that approach instead.

If you want to avoid npm, the fixtures will be on Github in a standard place for each component at: package/govuk/components/[component-name]/fixtures.json. I believe the package directory also contains a package.json which has the current version. Is that the kind of thing you were imagining?

Can you give a little more detail or an example of the backward-compatibility you mentioned?

@frankieroberto
Copy link
Contributor

@vanitabarrett thanks for your response.

We will generate fixtures for all components in GOVUK Frontend, so the fieldset and label will have fixtures. The fixtures are generated from the YAML files, so anything with a YAML file will get a corresponding fixtures file.

Great. Would these be under the existing examples key in the YAML, or are you proposing a separate key for the test fixtures? (This doesn’t affect people using the JSON file, but it'd be good to understand what the expectations are for contributors of new components)

Any components that support caller should also have another option, for example: passing HTML instead. We would suggest taking that approach instead.

That makes sense, but the govukFieldset component currently doesn't have an html option. The caller is technically optional, but without it you can only have a fieldset with a label, not any content.

You could either add an html option to the component, or another idea would be to have a special argument name for the caller, eg __caller__.

If you want to avoid npm, the fixtures will be on Github in a standard place for each component at: package/govuk/components/[component-name]/fixtures.json. I believe the package directory also contains a package.json which has the current version. Is that the kind of thing you were imagining?

Sounds good.

Can you give a little more detail or an example of the backward-compatibility you mentioned?

If the JSON file looked like this:

{
  "examples": [ 
    {
      "name": "default",
      "options": {
        "text": "Save and continue"
      },
      "html": "<button class=\"govuk-button\" data-module=\"govuk-button\">\n  Save and continue\n</button>"
    }
  ]
}

rather than this:

[ 
  {
    "name": "default",
    "options": {
      "text": "Save and continue"
    },
    "html": "<button class=\"govuk-button\" data-module=\"govuk-button\">\n  Save and continue\n</button>"
  }
]

then that allows you to add additional top-level keys to the JSON file, without breaking any existing code which is simply iterating through the examples array:

{
  "name": "Button",
  "url": "https://design-system.service.gov.uk/components/button/",
  "version": "3.8.0",
  "deprecated": false,
  "examples": [ 
    {
      "name": "default",
      "options": {
        "text": "Save and continue"
      },
      "html": "<button class=\"govuk-button\" data-module=\"govuk-button\">\n  Save and continue\n</button>"
    }
  ]
}

(It's hard to predict whether this’ll be needed or not, but may as well allow it as a possibility. It’s also recommended by the GDS API guidance, I just discovered)

@vanitabarrett
Copy link
Contributor

Thanks for clarifying @frankieroberto, I think your suggestions sound sensible! 👍

Great. Would these be under the existing examples key in the YAML, or are you proposing a separate key for the test fixtures? (This doesn’t affect people using the JSON file, but it'd be good to understand what the expectations are for contributors of new components)

Yes, the fixtures files will be generated from the examples in the YAML files. We’ll need to do a bit of work to ensure the current examples are comprehensive enough before generating the first set of fixtures. I think we’ll probably review the existing documentation for contributors to see if anything needs tweaking there too.

@vanitabarrett
Copy link
Contributor

This has been included in v3.9.0 of GOVUK Frontend 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
epic Epics are used in planning project boards to group related stories
Projects
None yet
Development

No branches or pull requests

9 participants