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

Flatten statutory sick pay tests and remove incorrect test context #1733

Conversation

Projects
None yet
3 participants
@floehopper
Copy link
Contributor

commented Jun 22, 2015

This PR is an attempt to merge the first two commits from #1634
(i.e. the statutory-sick-pay-updates branch) into master.

These two commits only involved changes to test and did not change the
behaviour of the system. More specifically the first commit removes an
intermediate-level test context and the second commit does a significant
flattening of the test's contexts.

Unfortunately this branch (which is out for Fact Check) has got quite
out-of-date with master and in particular these commits have a nasty
conflict with this commit in master.

By merging these two commits into master I hope to reduce the divergence
of this branch relative to master. Once these two commits have been merged
into master, I plan to rebase statutory-sick-pay-updates against master
and force-push it.

tadast added some commits May 12, 2015

Remove 'Already getting maternity allowance' context
It was incorrect: nested examples covered the cases where a user
is not getting maternity allowance

@floehopper says:

I've just cherry-picked this commit from @BenJanecke's
`statutory-sick-pay-updates` branch i.e. #1634.

In doing so I've had to resolve a bunch of conflicts in
`test/integration/smart_answer_flows/calculate_statutory_sick_pay_test.rb`
which were all due to a combination of the change in nesting of the contexts in the
original commit [1] versus the addition of some new contexts in [2].

[1]: 27d7b96
[2]: f3eea3b
Flatten statutory sick pay tests
Removing VERY nested context introduces some duplication in test
setup, but makes the tests easier to read, understand and change.

@floehopper says:

I've just cherry-picked this commit from @BenJanecke's
`statutory-sick-pay-updates` branch i.e. #1634.

In doing so I've had to resolve a bunch of conflicts in
`test/integration/smart_answer_flows/calculate_statutory_sick_pay_test.rb`
which were all due to a combination of the change in nesting of the contexts in
the original commit [1] versus the addition of some new date question
validation contexts in [2].

Rather than re-introduce a bunch of nested contexts, I've added top-level
contexts specifically to test the date question validation. This seems to fit
better with the new style being used in this test case.

[1]: 1b12656
[2]: f3eea3b
@floehopper

This comment has been minimized.

Copy link
Contributor Author

commented Jun 22, 2015

In reviewing this PR, @chrisroos spotted a mistake and so we've just paired on a different approach which he'll push shortly. I'm going to close this and delete the remote branch so @chrisroos can re-use the same branch name.

@floehopper floehopper closed this Jun 22, 2015

@floehopper floehopper deleted the flatten-statutory-sick-pay-tests-and-remove-incorrect-test-context branch Jun 22, 2015

@chrisroos

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2015

I've manually merged the new branch in b938a78.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.