Add get involved tests to replace Smokey tests #2473
Conversation
There was a problem hiding this comment.
Very nice 👏.
I've checked the tests against the page and they (now) line up, except for the new edge case I mentioned in a comment - not really worth blocking on though.
Some of the smoke tests are looking in finer detail than we are here (example). Much of this is static / declarative so I don't think we actually need to replicate it.
There is some functionality that's not covered at all e.g.
However, this is still a net improvement and we're at least implicitly testing much of the code doesn't break. I don't think it's worth adding more tests right now.
| } | ||
| stub_search(body) | ||
| stub_request(:get, /\A#{Plek.current.find('search')}\/search.json/) |
There was a problem hiding this comment.
How about DRYing up the URL into a variable?
It's a relatively complex sequence to read over and again.
There was a problem hiding this comment.
That's a good shout, I've set it as a helper method.
| @@ -53,14 +53,14 @@ | |||
| <%= render "govuk_publishing_components/components/big_number", { | |||
| number: @open_consultation_count, | |||
| label: t('get_involved.open_consultations'), | |||
| href: "/government/organisations#ministerial_departments" | |||
| href: "/search/policy-papers-and-consultations?content_store_document_type=open_consultations" | |||
There was a problem hiding this comment.
Great digging here - it's rare both the code and the test are broken!
kevindew
force-pushed
the
rewrite-get-involved-tests
branch
from
c9c338b to
b802b2d
Compare
|
Thanks @benthorner for the review. I've made a change and I'll plan to merge if it clears CI, shout if it raises any concerns. You're right we could do more, there is lots of nuances for this. I'd think they'd be ripe for a wider refactoring where presumably some tested helpers should be introduced. |
This changes the previous tests, which tested a number of controller methods, into a set of tests that take the more idiomatic Rails approach of testing the controllers via requests. The motivation for this work was to enable the retirement of a large set of tests for this set of pages done through Smokey [1]. These Smokey tests are unconventional as they seem to be used as a the primary means to test these pages - this is not a reliable or scalable approach given they're in a separate repository. Nor is it necessary as the logic is within this application. [1]: https://github.com/alphagov/smokey/blob/7553a432259d785fb6d4fb81cfd4fe3572185ad3/features/apps/government_frontend.feature#L23-L140
I intend to remove the majority of Smokey tests for this page so this warning will no longer be applicable.
These links were incorrect, presumably they are an artefact from what was used to copy+paste from originally. Interestingly these links actually have explicit tests in Smokey [1]. But it turns out they don't actually test correctly as they don't have an expect step [2]. As it is quite unusual to test at the fidelity of individual links in HTML (they're just testing you typed the same thing afterall) I've not added replacement tests. [1]: https://github.com/alphagov/smokey/blob/7553a432259d785fb6d4fb81cfd4fe3572185ad3/features/apps/government_frontend.feature#L23-L47 [2]: https://github.com/alphagov/smokey/blob/7553a432259d785fb6d4fb81cfd4fe3572185ad3/features/step_definitions/get_involved_page_steps.rb#L39-L41
kevindew
force-pushed
the
rewrite-get-involved-tests
branch
from
b802b2d to
fb039c6
Compare
Tests for the get-involved page have been written in government-frontend now [1], so we can now remove these features that don't meet the Smokey eligibility criteria. [1]: alphagov/government-frontend#2473
The /government/get-involved page was previously tested in a rather unusual way where there weren't any real tests for the endpoint in this application but there was an unusually thorough set of tests in Smokey. This isn't ideal because the Smokey tests are expensive and unnecessary since the logic is in this app.
This rewrites the tests to allow for the majority of the Smokey tests to be removed.