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

[Don't merge] Make test coverage data available for smart answer flows #1638

Closed

Conversation

floehopper
Copy link
Contributor

DO NOT MERGE

This is a "spike" which demonstrates how to obtain test coverage data for
the additional-commodity-code flow. This data was not previously
available
, because the flow files were being read and eval'ed rather than
simply required. The latter is necessary for SimpleCov to record coverage
data
for the file.

By extracting the existing DSL code into a define method on a subclass of
SmartAnswer::Flow, we can safely require the file separately from
instantiating the flow. This feels like a better state of affairs in general,
but it specifically addresses the test coverage problem above.

We'd really like to have this coverage data available so we can have confidence
that we're not breaking anything as we continue to refactor the app.

Note that I've intentionally not fixed the indentation in the
additional-commodity-code flow file so that it's easier to see my changes.

I think it would be simple enough to make this change for all smart answer
flows and then the code in SmartAnswer::FlowRegistry#build_flow could be
considerably simplified.

This is a "spike" which demonstrates how to obtain test coverage data for
the `additional-commodity-code` flow. This data was not previously
available [1], because the flow files were being read and eval'ed rather than
simply required. The latter is necessary [2] for `SimpleCov` to record coverage
data for the file.

By extracting the existing DSL code into a `define` method on a subclass of
`SmartAnswer::Flow`, we can safely require the file separately from
instantiating the flow. This feels like a better state of affairs in general,
but it specifically addresses the test coverage problem above.

We'd really like to have this coverage data available so we can have confidence
that we're not breaking anything as we continue to refactor the app.

Note that I've intentionally not fixed the indentation in the
`additional-commodity-code` flow file so that it's easier to see my changes.

[1]: https://ci-new.alphagov.co.uk/job/govuk_smart_answers/2396/rcov/
[2]: simplecov-ruby/simplecov#38
@floehopper floehopper changed the title Make test coverage data available for smart answer flows [Don't merge] Make test coverage data available for smart answer flows May 14, 2015
@tadast
Copy link
Contributor

tadast commented May 15, 2015

I don't mind if all the flows become ruby classes and we get rid of eval :) The biggest problem I see is it will make it a bit more difficult to look at the git history before this change, since we'll need to indent all the code in flows.

Is SimpleCov smart enough to measure test coverage in a DSL like ours? If so and if you trust those stats, then it's a worthwhile change perhaps :)

@floehopper
Copy link
Contributor Author

@tadast:

@chrisroos
Copy link
Contributor

I'm very much in favour of this!

We're hoping to introduce some fairly comprehensive regression tests around the existing smart answers. Simplecov helps us by allowing us to build up the smallest set of inputs that exercise most/all of the flows.

@tadast
Copy link
Contributor

tadast commented May 18, 2015

LGTM then, an extra step when browsing git history is a small price to pay for increased confidence :) 👍

@floehopper
Copy link
Contributor Author

I've just made these changes in a separate branch and merged it in this commit.

Note that I have purposefully not yet changed the following:

  • calculate-employee-redundancy-pay
  • calculate-your-redundancy-pay
  • calculate-statutory-sick-pay
  • marriage-abroad

Closing this PR which was just a spike.

@floehopper floehopper closed this May 18, 2015
@floehopper floehopper deleted the make-test-coverage-available-for-smart-answer-flows branch May 18, 2015 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants