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

Update regression test input YAML to current YAML version #2011

Merged
merged 4 commits into from Oct 15, 2015

Conversation

Projects
None yet
2 participants
@floehopper
Contributor

floehopper commented Oct 15, 2015

The version of YAML has changed since these files were generated and the
newer version seems to generate slightly different YAML output e.g. it uses
single-quotes instead of double-quotes.

Because of this, when we make a change and regenerate these files, it's
difficult to see the woods for the trees. By loading and saving all the YAML
files in this PR, I hope that it will be easier to see what's changed in
later commits.

@chrisroos

This comment has been minimized.

Show comment
Hide comment
@chrisroos

chrisroos Oct 15, 2015

Contributor

Loading and saving all the files has removed some of the comments that were added to describe why certain values were chosen:

  • test/data/estimate-self-assessment-penalties-questions-and-responses.yml
  • test/data/help-if-you-are-arrested-abroad-questions-and-responses.yml
  • test/data/landlord-immigration-check-questions-and-responses.yml
  • test/data/legalisation-document-checker-questions-and-responses.yml
  • test/data/maternity-paternity-calculator-questions-and-responses.yml
  • test/data/overseas-passports-questions-and-responses.yml
  • test/data/register-a-birth-questions-and-responses.yml
  • test/data/register-a-death-questions-and-responses.yml
  • test/data/simplified-expenses-checker-questions-and-responses.yml
  • test/data/state-pension-topup-questions-and-responses.yml
  • test/data/uk-benefits-abroad-questions-and-responses.yml

Do you think we should try to retain those?

Contributor

chrisroos commented Oct 15, 2015

Loading and saving all the files has removed some of the comments that were added to describe why certain values were chosen:

  • test/data/estimate-self-assessment-penalties-questions-and-responses.yml
  • test/data/help-if-you-are-arrested-abroad-questions-and-responses.yml
  • test/data/landlord-immigration-check-questions-and-responses.yml
  • test/data/legalisation-document-checker-questions-and-responses.yml
  • test/data/maternity-paternity-calculator-questions-and-responses.yml
  • test/data/overseas-passports-questions-and-responses.yml
  • test/data/register-a-birth-questions-and-responses.yml
  • test/data/register-a-death-questions-and-responses.yml
  • test/data/simplified-expenses-checker-questions-and-responses.yml
  • test/data/state-pension-topup-questions-and-responses.yml
  • test/data/uk-benefits-abroad-questions-and-responses.yml

Do you think we should try to retain those?

@floehopper

This comment has been minimized.

Show comment
Hide comment
@floehopper

floehopper Oct 15, 2015

Contributor

Oops! Yes. I'll sort that now. Well spotted!

Contributor

floehopper commented Oct 15, 2015

Oops! Yes. I'll sort that now. Well spotted!

@floehopper

This comment has been minimized.

Show comment
Hide comment
@floehopper

floehopper Oct 15, 2015

Contributor

I've updated the middle "Update YAML files using current YAML version" commit to include manually re-adding the comments and updated the commit message to explain how I did it. I've then force-pushed.

Contributor

floehopper commented Oct 15, 2015

I've updated the middle "Update YAML files using current YAML version" commit to include manually re-adding the comments and updated the commit message to explain how I did it. I've then force-pushed.

@chrisroos

This comment has been minimized.

Show comment
Hide comment
@chrisroos

chrisroos Oct 15, 2015

Contributor

Looks good to me, although presumably you'll need to regenerate all the checksum data too?

Contributor

chrisroos commented Oct 15, 2015

Looks good to me, although presumably you'll need to regenerate all the checksum data too?

@floehopper

This comment has been minimized.

Show comment
Hide comment
@floehopper

floehopper Oct 15, 2015

Contributor

Yes, I'll update checksums now and get the PR merged. Thanks.

Contributor

floehopper commented Oct 15, 2015

Yes, I'll update checksums now and get the PR merged. Thanks.

@floehopper

This comment has been minimized.

Show comment
Hide comment
@floehopper

floehopper Oct 15, 2015

Contributor

I've just added a commit which updates the checksums and force-pushed in preparation for merging.

Contributor

floehopper commented Oct 15, 2015

I've just added a commit which updates the checksums and force-pushed in preparation for merging.

floehopper added some commits Oct 15, 2015

Update YAML files using current YAML version
The version of YAML has changed since these files were generated and the newer
version seems to generate slightly different YAML output e.g. it uses single-
quotes instead of double-quotes.

Because of this, when we make a change and regenerate these files, it's
difficult to see the woods for the trees. By loading and saving all the YAML
files in this commit, I hope that it will be easier to see what's changed in
later commits.

Some of the `*-questions-and-responses.yml` files included comments which
would've been lost in this automated process. So I ran the following command
to identify them:

    grep -R "#" test/data/*

I then ran the following command to do the automatic update:

    rails r script/update-yaml.rb

And finally I manually edited the files to re-add the comments identified
above. I re-ran the `grep` command to check I hadn't missed any comments.
Revert "TEMP: Add update-yaml script"
This reverts commit edcd63e.

Now that I've used this script it is surplus to requirements.
@floehopper

This comment has been minimized.

Show comment
Hide comment
@floehopper

floehopper Oct 15, 2015

Contributor

I've just rebased this against master and force-pushed in preparation for merging.

Contributor

floehopper commented Oct 15, 2015

I've just rebased this against master and force-pushed in preparation for merging.

floehopper added a commit that referenced this pull request Oct 15, 2015

Merge pull request #2011 from alphagov/update-regression-test-input-y…
…aml-to-current-yaml-version

Update regression test input YAML to current YAML version

@floehopper floehopper merged commit 35525cf into master Oct 15, 2015

1 check passed

default "Build #3112 succeeded on Jenkins"
Details

@floehopper floehopper deleted the update-regression-test-input-yaml-to-current-yaml-version branch Oct 15, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment