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

Refactor unit test for SmartAnswer::Question::Base #2340

Merged
merged 12 commits into from Mar 8, 2016

Conversation

Projects
None yet
2 participants
@floehopper
Copy link
Contributor

commented Mar 7, 2016

This follows on from #2319 and is further work in preparation for a proper version of #2311.

I was finding it increasingly hard to read and edit the unit test for SmartAnswer::Question::Base, so I decided to to some preparatory refactoring.

The main change in this PR is the grouping together of related tests into Shoulda contexts. I hope the commits are small enough that you can see what's going on in the diffs - you might need to ignore whitespace changes to make things clearer. I've also extracted a method to DRY up the test code a bit.

See the individual commit notes for details.

Given that I've only made changes to a single unit test and no "production" code, these changes should be pretty low risk, but it would be good if someone could give them the once over.

floehopper added some commits Mar 7, 2016

Use should vs test to define tests for Question::Base
I want to introduce `shoulda` contexts into this test to make it easier
to understand, but these don't appear to work in conjunction with
`ActiveSupport`-style test definitions.
@erikeide

This comment has been minimized.

Copy link
Contributor

commented on c35f8c8 Mar 7, 2016

Do we ever do anything with the messages of the exception? If not, I wonder if there is much benefit in testing the message value, and if the duplication is creating an unnecessary coupling

This comment has been minimized.

Copy link
Contributor Author

replied Mar 7, 2016

I don't think we ever rescue this exception, so in that sense the only place the message is used is in Errbit reporting or whatever. I think there is some value in testing this message from the point of view that it is doing String interpolation, but I think you're right that we probably don't need to test the message in both of these tests. That in turn would mean that we wouldn't need so much setup for the one in which we don't test the message.

This comment has been minimized.

Copy link
Contributor Author

replied Mar 8, 2016

I've addressed this in a new commit: 3271c8b.

@erikeide

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2016

Other than minor comments, looks good to me

@floehopper floehopper force-pushed the refactor-question-base-test branch from 47f773e to 5b1987e Mar 8, 2016

@floehopper

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2016

I'm going to do the interactive rebase against master now and force push in preparation for merging.

floehopper added some commits Mar 7, 2016

Move test for Question::Base#permitted_next_nodes
Into a context and improve the test name.
Improve test coverage for Question::Base#permitted_next_nodes
I noticed this scenario was missing after I'd reorganised the tests into
`shoulda` contexts.
Construct question in a setup block
This removes some duplication, improves the signal-to-noise ratio,
and makes it clearer which is the method-under-test in each case.
Remove redundant line from Question::Base test
The call to the constructor of `SmartAnswer::State` in the previous line sets
its `current_node` to this value anyway.
Only test NextNodeUndefined exception message once
There were two tests asserting against the message of this exception.

While it's worthwhile testing that both scenarios raise the exception,
it's not worth asserting against the message in both tests.

@floehopper floehopper force-pushed the refactor-question-base-test branch from 5b1987e to 3103862 Mar 8, 2016

floehopper added a commit that referenced this pull request Mar 8, 2016

Merge pull request #2340 from alphagov/refactor-question-base-test
Refactor unit test for SmartAnswer::Question::Base

@floehopper floehopper merged commit 6942d51 into master Mar 8, 2016

1 check passed

default "Build #4331 succeeded on Jenkins"
Details

@floehopper floehopper deleted the refactor-question-base-test branch Mar 8, 2016

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.