Fix responses and expected results files for regression tests #2524

floehopper · 2016-05-12T17:34:40Z

Description

While looking into something else, I noticed that the *-responses-and-expected-results.yml files for a number of flows were out-of-date:

check-uk-visa
student-finance-forms
state-pension-through-partner
overseas-passports

This PR updates those files and identifies the PR in which they should have been updated in the first place. I haven't done any checking that these changes are sensible, but I plan to ask the authors of the relevant PRs to check for me, because they will have more context.

While fixing the above, I also came across a few other problems:

The changes in Simplify Services.worldwide_api method #2498 meant that the scripts for generating both the "questions & responses" and "responses & expected results" files were trying to use a local instance of the Whitehall app rather than the production instance.
The questions & responses file for simplified-expenses-checker contained some invalid YAML.
The questions & responses file for landlord-immigration-check was not up-to-date with the stubbed imminence lookups in the regression test nor its responses & expected results file.

I've fixed all of the above and it would be good to get this merged as soon as possible.

Note that I've also noticed that the script for generating the responses & expected results is using the wrong stubbed value of current time for some flows. I plan to address that in a separate PR, but that PR will rely on this one.

External changes

None. These changes only affect test code - no changes to production code.

This script runs in the Rails `development` environment. Prior to #2498, being in the `development` environment meant that the app always used the production instance of the Whitehall app as the backend for the GDS Worldwide API. After #2498, by default the app would try to use a local development instance of the Whitehall app, unless the `PLEK_SERVICE_WHITEHALL_ADMIN_URI` is set. Since the existing `*-responses-and-expected-results.yml` files were generated with the production instance of the Whitehall app, it seems more sensible to set the `PLEK_SERVICE_WHITEHALL_ADMIN_URI` environment variable to ensure that this continues to be the case.

It looks like the YAML was accidentally broken in this commit [1]. Also updated regression test checksums, because all the regression tests are passing after the changes to the questions and responses file. [1]: 853eeed

It looks as if this was intended to be done in #2256. More specifically it looks as if it was missed out of this commit [1]. Also updated regression test checksums, because all the regression tests are passing after the changes to the questions and responses file. [1]: 4801da6

These changes should have been made in #2289. Also updated regression test checksums, because all the regression tests are passing after the changes to the responses and expected results file.

These changes should have been made in #2296. Also updated regression test artefacts in the light of these changes and then updated regression test checksums, because all the regression tests are now passing.

These changes should have been made in #2432. Also updated regression test artefacts in the light of these changes and then updated regression test checksums, because all the regression tests are now passing.

These changes should have been made in #2463. Also updated regression test checksums, because all the regression tests are passing after the changes to the responses and expected results file.

chrisroos · 2016-05-13T04:15:54Z

script/generate-responses-and-expected-results-for-smart-answer.rb

@@ -1,3 +1,5 @@
+ENV['PLEK_SERVICE_WHITEHALL_ADMIN_URI'] = 'https://www.gov.uk'


It looks as though this line should be added to generate-questions-and-responses-for-smart-answer.rb too. It's not clear to me whether that should happen in this PR or not though.

Good point. I think I might as well add it to this PR.

chrisroos · 2016-05-13T04:22:59Z

Apart from my single comment, this all looks good to me.

chrisroos · 2016-05-13T04:24:52Z

I wonder whether it's worth adding tests to avoid these sort of problems in future? Do you think the effort of adding such tests would be warranted?

floehopper · 2016-05-13T11:08:42Z

@chrisroos:

I wonder whether it's worth adding tests to avoid these sort of problems in future? Do you think the effort of adding such tests would be warranted?

I'm not sure. It's not obvious to me how these changes were missed. I suspect that because many PRs only involve content changes people may just get into the habit of only updating the regression test artefacts & checksums, and not think about whether these files need updating.

I think the first thing to do would be to check that the documentation makes it clear when you should update these files. Then we could consider adding some kind of check or test to make sure they do get updated. I did wonder whether we could just always run the generate script before running the regression test for that flow. Anyway, I think it's out of scope for this PR, but I've created a Trello card to capture the idea.

floehopper · 2016-05-13T11:14:57Z

test/data/check-uk-visa-responses-and-expected-results.yml

@@ -630,7 +630,7 @@
  - kuwait
  - work
  - six_months_or_less
-  :next_node: :outcome_work_m
+  :next_node: :outcome_work_waiver


It looks as if this change was the result of this commit: 8053b80 which was part of #2524. I'm therefore happy to include it.

floehopper · 2016-05-13T11:55:04Z

test/data/overseas-passports-responses-and-expected-results.yml

@@ -752,7 +752,7 @@
 - :current_node: :which_country_are_you_in?
  :responses:
  - yemen
-  :next_node: :cannot_apply
+  :next_node: :apply_in_neighbouring_country


I can see that this change was caused by this commit: 9c71c56 as part of #2463. So I'm happy to include it in this PR.

floehopper · 2016-05-13T11:59:00Z

I'm going to rebase this against master and open a fresh PR so as not to disrupt the comments on this one.

floehopper · 2016-05-13T12:07:10Z

Superseded by #2527. Closing.

floehopper added 7 commits May 12, 2016 15:20

Fix questions & responses for simplified-expenses-checker

2cbd51c

It looks like the YAML was accidentally broken in this commit [1]. Also updated regression test checksums, because all the regression tests are passing after the changes to the questions and responses file. [1]: 853eeed

Update responses and expected results for check-uk-visa

97e4c2a

These changes should have been made in #2289. Also updated regression test checksums, because all the regression tests are passing after the changes to the responses and expected results file.

Update responses and expected results for student-finance-forms

6b94eb9

These changes should have been made in #2296. Also updated regression test artefacts in the light of these changes and then updated regression test checksums, because all the regression tests are now passing.

Update responses and expected results for state-pension-through-partner

a3d2a69

These changes should have been made in #2432. Also updated regression test artefacts in the light of these changes and then updated regression test checksums, because all the regression tests are now passing.

Update responses and expected results for overseas-passports

11fe3b4

These changes should have been made in #2463. Also updated regression test checksums, because all the regression tests are passing after the changes to the responses and expected results file.

This was referenced May 12, 2016

State Pension Through Partner's National Insurance Record #2432

Merged

Overseas passports content changes to no service countries and Getting your passport #2463

Merged

floehopper added the Refactoring label May 12, 2016

chrisroos reviewed May 13, 2016
View reviewed changes

chrisroos added the LGTM label May 13, 2016

chrisroos self-assigned this May 13, 2016

floehopper reviewed May 13, 2016
View reviewed changes

This was referenced May 13, 2016

Update outcomes for countries in Electronic Visa Waiver program #2289

Merged

Update Student finance forms with 2016 - 2017 option for full time students #2296

Merged

floehopper reviewed May 13, 2016
View reviewed changes

floehopper mentioned this pull request May 13, 2016

Fix responses and expected results files after feedback #2527

Merged

floehopper closed this May 13, 2016

floehopper deleted the fix-responses-and-expected-results-files branch June 1, 2016 16:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix responses and expected results files for regression tests #2524

Fix responses and expected results files for regression tests #2524

floehopper commented May 12, 2016 •

edited

Loading

chrisroos May 13, 2016

floehopper May 13, 2016

chrisroos commented May 13, 2016

chrisroos commented May 13, 2016

floehopper commented May 13, 2016

floehopper May 13, 2016

floehopper May 13, 2016

floehopper commented May 13, 2016

floehopper commented May 13, 2016

		@@ -1,3 +1,5 @@
		ENV['PLEK_SERVICE_WHITEHALL_ADMIN_URI'] = 'https://www.gov.uk'

Fix responses and expected results files for regression tests #2524

Fix responses and expected results files for regression tests #2524

Conversation

floehopper commented May 12, 2016 • edited Loading

Description

External changes

chrisroos May 13, 2016

Choose a reason for hiding this comment

floehopper May 13, 2016

Choose a reason for hiding this comment

chrisroos commented May 13, 2016

chrisroos commented May 13, 2016

floehopper commented May 13, 2016

floehopper May 13, 2016

Choose a reason for hiding this comment

floehopper May 13, 2016

Choose a reason for hiding this comment

floehopper commented May 13, 2016

floehopper commented May 13, 2016

floehopper commented May 12, 2016 •

edited

Loading