Update regression tests first for marriage-abroad Denmark change

[chrisroos]

This PR demonstrates an approach to making marriage-abroad content changes by updating the regression test artefacts first.

We currently make these sort of changes by updating the flows/templates and then regenerating the regression test artefacts to confirm that the changes have had the desired effect. One of the problems with this approach is that it's not always easy/possible to see the effect of the changes by looking at the diff of the regression test artefacts. This, in combination with the overly complicated marriage-abroad code, makes it possible to commit unintentional changes to the rendered outcomes.

Updating the regression test artefacts first, and then updating the flows/templates, gives us confidence that the changes we make only affect the things we want.

I chose a recent marriage-abroad content change (PR #2576) and have reimplemented it in this branch by updating the regression test artefacts first.

The branch contains a number of preparatory commits that aren't interesting when it comes to understanding this approach. These commits refactor the code (i.e. don't change behaviour and therefore don't result in changes to the regression test artefacts) to make the actual change easier.

The last 6 commits are the only ones of real interest:

1. Update marriage-abroad denmark/uk opposite sex test artefacts (72a4bd4)
   • Manually updates 3 regression test artefacts based on the requirements in the Google Doc.
   • Results in 3 failing regression tests. For paths containing "denmark/uk/*/opposite_sex".
2. Update marriage-abroad denmark/ceremony_country opposite sex test artefacts (1592f2b)
   • Manually updates 3 regression test artefacts based on the requirements in the Google Doc
   • Results in 3 more failing regression tests. For paths containing "denmark/ceremony_country/*/opposite_sex".
3. Make changes required by opposite sex cereonies in Denmark (de3b909)
   • Update the outcome template to fix the 6 failing regression tests.
4. Update marriage-abroad denmark/uk same-sex test artefacts (59d2c77)
   • Manually updates 3 regression test artefacts based on the requirements in the Google Doc.
   • Results in 3 failing regression tests. For paths containing "denmark/uk/*/same_sex".
5. Update marriage-abroad denmark/ceremony_country same-sex test artefacts (a38a3a6)
   • Manually updates 3 regression test artefacts based on the requirements in the Google Doc.
   • Results in 3 more failing regression tests. For paths containing "denmark/ceremony_country/*/same_sex".
6. Make changes for same sex ceremonies in Denmark (42a4244)
   • Update the flow, data and outcome to fix the 6 failing regression tests.

I think there's an additional benefit of this approach in that I can imagine helping the content team/department to make changes to the regression test artefacts directly; allowing us to remove the intermediate step of translating the requirements in a Google Doc.

What do you think, @ikennaokpala, @pmanrubia, @leenagupte and @floehopper? Does this seem like a good approach to you?

Note that while I've focussed on marriage-abroad, I can't see any reason we can't use this approach for all these sort of content changes.

[leenagupte]

@chrisroos I'm confused. Perhaps you could make the commit statements match the bullet points in the PR description. I don't see how you have update the test artefacts before changing the code if you aren't adding Denmark as another option to be tested in the questions and responses YAML file.

[chrisroos]

Hey @leenagupte.

Denmark was already in marriage-abroad-questions-and-responses.yml which is why I didn't have to add it in this branch.

I don't quite follow what you mean by making "the commit statements match the bullet points in the PR description". The 6 top-level bullet points in the PR description contain the titles of, and links to, commits in this PR. The indented bullet points below each of those 6 points contain a summary of the commit.

I've just updated the top level bullet points to a numbered list (1 to 6) in case that helps make it clearer that these represent the 6 commits I talk about in the description.

I've also amended the commit notes of those 6 commits:

• In commits 1 and 2, I've made it clearer that each of them breaks 3 regression tests.
• In commit 3, I've made it clearer that it fixes the 6 broken regression tests introduced in commits 1 and 2.
• In commits 4 and 5, I've made it clearer that each of them breaks 3 regression tests.
• In commit 6, I've made it clearer that it fixes the 6 broken regression tests introduced in commits 4 and 5.

Does any of that make things any clearer?

[leenagupte]

@chrisroos This statement is confusing as the Regresssion test artefacts are not updated until commit 19.:

Updating the regression test artefacts first, and then updating the flows/templates, gives us confidence that the changes we make only affect the things we want.

[chrisroos]

@leenagupte: I've updated the PR description in the hope that it's clearer that you can ignore all the preparatory commits. These just refactor the code in order to make the actual change easier to implement.

The approach I'm trying to describe/promote in this branch can be seen in the last 6 commits, which are all linked from the description.

Does that help?

@chrisroos This statement is confusing as the Regresssion test artefacts are not updated until commit 19.:

Updating the regression test artefacts first, and then updating the flows/templates, gives us confidence that the changes we make only affect the things we want.

[leenagupte]

@chrisroos My confusion stems from the fact that you are manually updating the test artefacts.

That's fine if you only have to change 3 artefacts, but a change I made to a partial template used in outcome page affected over 2,500 test artefacts and it would be madness to have updated all of those manually. See PR: #2427

Unless, are you actually just committing the changes in reverse order?
i.e.

1. updating the templates
2. regenerating the test artefacts
3. commit the test artefacts
4. commit the templates

[chrisroos]

@chrisroos My confusion stems from the fact that you are manually updating the test artefacts.

That's fine if you only have to change 3 artefacts, but a change I made to a partial template used in outcome page affected over 2,500 test artefacts and it would be madness to have updated all of those manually. See PR: #2427

Ah, OK. I should've made it clearer that this approach almost certainly won't work for all types of content changes. Although, having just looked, I do think it would've been possible to use this test-first approach in PR #2427. By automatically replacing all occurrences of the removed line ("You can [claim back sick pay]...") from the test artefacts, we would've been left with failing regression tests that can then be fixed by removing the same line from "entitled_to_sick_pay.govspeak.erb".

Unless, are you actually just committing the changes in reverse order?
i.e.

1. updating the templates
2. regenerating the test artefacts
3. commit the test artefacts
4. commit the templates

Nope. The order is:

1. Update regression test artefacts so that they contain the results of the content changes. This gives us a failing regression test.
2. Update the Smart Answer flow/templates to fix the failing test introduced in 1.

The current workflow (at least for marriage-abroad) is something like:

1. Content team/department describe changes to outcomes in a Google Doc.
2. Developers make the changes to the system that we think will result in the changes described in the Google Doc.
3. Developers inspect the changes to the regression test artefacts to confirm whether the changes to the system have had the desired effect.

The main problem I'm trying to solve with the approach in this PR is to avoid having to carefully review/explain the changes to regression test artefacts in step 3. By updating the artefacts first, and then changing the system until the tests pass, we can be confident that our changes are correct.

Does that make it any clearer, @leenagupte?

[leenagupte]

By automatically replacing all occurrences of the removed line ("You can [claim back sick pay]...") from the test artefacts

Automatically how? Relying on your IDE to do a find and replace?

The problem is that there are so many test artefacts generated. And in the case of #2427 it's impossible for a human being to review them all. Either as the developer or the code reviewer.

I think that we should take into account when a change modifies the flow and when they just update the copy.

I think that for flow changes, or more specifically, adding a new option, then yes, update the questions and responses file first and then regenerate the test artefacts and start with failing tests, but for parts of the change that are just changes to the copy in templates, it's fine to do those changes first, and the regenerate the test artefacts to match them.

The copy in the templates is not as not as important to me as the structure of the file path that used to mimic the order of the questions and the URL.

[pmanrubia]

Hi @chrisroos, I find it very interesting. Content designers are more used to work with documents than with reusable templates.

So..., two nice wins here:

1. If a Content designer edits the artefacts: they would just need to update text documents in markdown. It fits their mental model and their normal procedures.
2. If a developer edits the artefact: we have a failing test that matches the document provided by the content designer. No need to explain the artefacts, and we would save time.

I am happy to explore this approach for incoming PRs, but I would delay it a few weeks; the main reason is that we would need to get up to speed in creating PRs, organising commits / PRs, tests and delivering stories. Starting a new exploratory path right now seems a bit too early for me, to be honest. Does it sound ok to you?

As a side note, It would be worth discussing about:

1. This idea might be related to this Trello card: https://trello.com/b/bVfnXnEc/content-development. A tool to get the list of the pages / documents / artefacts that would need to be modified when updating a flow.
2. I guess we all agree that this new approach is not ideal when changes could impact in dozens, hundreds of files (another argument in favor of what I described above)
3. Current coverage of our artefacts.

So to summarise my feeling about it:

1. A very interesting idea to explore that could apply to a subset of the smart-answers considering the tools we have so far.
2. Happy to give it try in a few weeks once we are up to speed.

When it comes to involving content designers, I would try first with developers until the process is fully understood and agreed among all of us.

[floehopper]

@leenagupte:

By automatically replacing all occurrences of the removed line ("You can [claim back sick pay]...") from the test artefacts

Automatically how? Relying on your IDE to do a find and replace?

I imagine Chris means that you would craft one or more command lines (e.g. using sed) to do the replacement in an automated way.

The problem is that there are so many test artefacts generated. And in the case of #2427 it's impossible for a human being to review them all. Either as the developer or the code reviewer.

If the commands mentioned above were included in the commit note, then there would be no need to inspect the changes in the artefacts. The reviewer could simply re-run the command(s) against the original source and check that the result is the same as that in the commit.

[floehopper]

@pmanrubia: I agree with your assessment. It would be great if you could try this out in another card in the next few weeks.

[floehopper]

@chrisroos: This looks good to me. It feels as if it's an extension of what I described in the Preparatory commits section of the Regression tests documentation. If I get that merged, it might be worth extending or adding to that documentation to suggest this as an alternative approach.

[pmanrubia]

@chrisroos shall we keep this PR opened? I guess it served its purpose.
As a side note, a PR #2628 following the suggested approach

[chrisroos]

@chrisroos shall we keep this PR opened? I guess it served its purpose.

I agree that it's served its purpose. I'll close it now.

As a side note, a PR #2628 following the suggested approach

This is great! I'm glad you found the approach useful.