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

Avoid using worldwide api helpers in integration tests #2540

Merged

Conversation

@chrisroos
Copy link
Contributor

chrisroos commented May 19, 2016

This supersedes PR #2532. I've rebased the branch on master and am opening this ready to merge. No review required.

Trello card: https://trello.com/c/T69SfCaC

Stubbing WorldLocation.all and WorldLocation.find, instead of using the GdsApi::TestHelpers::Worldwide helper methods, appears to shave about 50-60 seconds off the time it takes to run rake (i.e. all but the regression tests) on my machine.

We're still using the GdsApi::TestHelpers::Worldwide methods in the country_and_date_questions_test, the regression tests and a couple of unit tests. I had hoped that making a similar change to the regression tests would give us a similar performance improvement but that doesn't seem to be the case based on my initial investigation. I plan to dig into this further at another time. I haven't updated the country_and_date_questions_test because making the same change caused the tests to fail. I didn't investigate in any detail but I think this might because of the JavaScript tests in that file. I didn't update the unit tests because I think this is a good enough improvement to get merged without spending more time on the branch.

chrisroos added 9 commits May 16, 2016
I'm going to use this method instead of
`GdsApi::TestHelpers::Worldwide#worldwide_api_has_locations` in the integration
tests. This appears to shave about a minute off the time it takes to run `rake`
(i.e. all but the regression tests) on my machine.

I'm committing this first and will commit the change to each integration test
separately.

This works by stubbing `WorldLocation.all` and `WorldLocation.find` to return
some stub `WorldLocation` objects. For each of those objects I've had to stub
`#slug`, `#name` and `#fco_organisation` to ensure that all the tests are
happy.

This is in contrast to `#worldwide_api_has_locations` which works by
configuring WebMock to return JSON data in response to requests to the GOV.UK
worldwide API. Using `#worldwide_api_has_locations` means that we're relying on
the real implementation of `GdsApi::Worldwide` to request the JSON and turn it
into `OpenStruct`'s to be consumed within `WorldLocation.all` and
`WorldLocation.find`.
I've also removed calls to `#worldwide_api_has_organisations_for_location`.
These are no longer required now that we're stubbing
`WorldLocation#fco_organisation` directly. They were previously required as
`WorldLocation#fco_organisation` calls `WorldLocation#organisations` which in
turn uses `GdsApi::Worldwide` to request JSON data from the GOV.UK worldwide
API.
I've removed calls to `#worldwide_api_has_organisations_for_location`.  These
are no longer required now that we're stubbing `WorldLocation#fco_organisation`
directly. They were previously required as `WorldLocation#fco_organisation`
calls `WorldLocation#organisations` which in turn uses `GdsApi::Worldwide` to
request JSON data from the GOV.UK worldwide API.

I've removed calls to `#worldwide_api_has_no_organisations_for_location`.  This
would've previously resulted in `WorldLocation#fco_organisation` returning
`nil` (because `WorldLocation#organisations` would've been an empty array).
It's no longer required because our `WorldLocation` stubs always return `nil`
for `#fco_organisation`.

I've removed two assertions that were failing after the change to use
`stub_worldwide_locations`.

The first was an assertion checking that `#country_name_lowercase_prefix`
matched 'the Czech Republic'. This was failing because the `WorldLocation#name`
returns the humanized version of the slug ('Czech-republic') rather than the
actual name of the country. I'm happy to delete this as I was in two minds
about keeping it when I changed it in c02bc47.

The second was an assertion checking that `fco_organisation.title` matches
'British Embassy San Jose'. The `WorldLocation` stubs we're using always return
`nil` for `#fco_organisation` which meant that this failed with a
`NoMethodError`. I'm happy to delete this as I was in two minds about keeping
it when I changed it in 28cea19.
I've removed calls to `#worldwide_api_has_organisations_for_location`.  These
are no longer required now that we're stubbing `WorldLocation#fco_organisation`
directly. They were previously required as `WorldLocation#fco_organisation`
calls `WorldLocation#organisations` which in turn uses `GdsApi::Worldwide` to
request JSON data from the GOV.UK worldwide API.

I've removed calls to `#worldwide_api_has_no_organisations_for_location`.  This
would've previously resulted in `WorldLocation#fco_organisation` returning
`nil` (because `WorldLocation#organisations` would've been an empty array).
It's no longer required because our `WorldLocation` stubs always return `nil`
for `#fco_organisation`.
I've removed calls to `#worldwide_api_has_organisations_for_location`.  These
are no longer required now that we're stubbing `WorldLocation#fco_organisation`
directly. They were previously required as `WorldLocation#fco_organisation`
calls `WorldLocation#organisations` which in turn uses `GdsApi::Worldwide` to
request JSON data from the GOV.UK worldwide API.

I've removed calls to `#worldwide_api_has_no_organisations_for_location`.  This
would've previously resulted in `WorldLocation#fco_organisation` returning
`nil` (because `WorldLocation#organisations` would've been an empty array).
It's no longer required because our `WorldLocation` stubs always return `nil`
for `#fco_organisation`.

There were two assertions failing after the switch to use `stub_worldwide_locations`.

The first was asserting that the `registration_country_name_lowercase_prefix`
state variable was 'Sri Lanka'. This fails because the `WorldLocation#name`
returns the humanized version of the slug ('Sri-lanka') rather than the actual
name of the country. I don't think this assertion was adding much value so I'm
happy to simply delete it.

The other failure was in the test named "lead to the North Korea-specific
result if the user is still there". I've chosen to fix this by changing the
implementation of the `overseas_passports_embassies` precalculate block in the
`north_korea_result` question of the register-a-birth flow. The test was
failing because our stub `WorldLocation` object returns `nil` for
`#fco_organisation`. I believe the code in the precalculate block was
attempting to guard against this situation by calling `organisations.present?`.
Unfortunately this returns true when `organisations` is `[nil]`. There are two
other `overseas_passports_embassies` precalculate blocks in this flow and they
both use `organisations && organisations.any?` to handle the situation of
`#fco_organisation` returning `nil`. I've updated this version to use the same
implementation. The register-a-birth flow can certainly be improved further but
that's out of scope of what I'm trying to do here.

I ran the register-a-birth regression tests after making this change to the
flow. They all pass so I'm happy to update the checksum data in this commit.
I've removed calls to `#worldwide_api_has_organisations_for_location`.  These
are no longer required now that we're stubbing `WorldLocation#fco_organisation`
directly. They were previously required as `WorldLocation#fco_organisation`
calls `WorldLocation#organisations` which in turn uses `GdsApi::Worldwide` to
request JSON data from the GOV.UK worldwide API.
I've removed calls to `#worldwide_api_has_organisations_for_location`.  These
are no longer required now that we're stubbing `WorldLocation#fco_organisation`
directly. They were previously required as `WorldLocation#fco_organisation`
calls `WorldLocation#organisations` which in turn uses `GdsApi::Worldwide` to
request JSON data from the GOV.UK worldwide API.

There was a single assertion failure after switching to
`stub_worldwide_locations`. The assertion was checking for 'British Embassy
Baku' in the body of the outcome. This now fails because our `WorldLocation`
stubs always return `nil` for `#fco_organisation`. I don't think the test is
particularly useful to I've deleted it rather than try to fix it.
@chrisroos chrisroos merged commit c71d9c3 into master May 19, 2016
1 check passed
1 check passed
default "Build #5163 succeeded on Jenkins"
Details
@chrisroos chrisroos deleted the avoid-using-worldwide-api-helpers-in-integration-tests branch May 19, 2016
chrisroos added a commit that referenced this pull request Jun 21, 2016
Trello card: https://trello.com/c/Hiizo5g6

This follows on from the work in PRs #2540 and #2555 and applies the same
changes to the regression tests.

Switching from WebMock to Mocha appears to reduce the time taken to run all
regression tests by about 15 minutes on my machine.

== Regression test duration before this commit:

    $ time RUN_REGRESSION_TESTS=true \
    ruby -Itest test/regression/smart_answers_regression_test.rb

    real  56m34.146s
    user  45m49.017s
    sys   9m19.234s

== Regression test duration after this commit:

    $ time RUN_REGRESSION_TESTS=true \
    ruby -Itest test/regression/smart_answers_regression_test.rb

    real  36m29.928s
    user  27m45.692s
    sys   8m33.085s

== Notes

The main difference between this and PRs #2540 and #2555 is that a number of
the regression tests rely on `WorldLocation#fco_organisation` returning a
`WorldwideOrganisation` rather than nil. I've satisfied this requirement by
adding the optional `load_fco_organisation_data` parameter to both
`#stub_world_locations` and `#stub_world_location`, and loading the FCO data
when the parameter is true.

The implementation in `#stub_world_location` uses the organisation data that
was previously being used in `#setup_worldwide_locations`. I've reimplemented
the logic from `WorldwideOrganisation#fco_sponsored?` by querying the
`organisations_data` hash. On finding an FCO organisation in the data I use
`GdsApi::Response.build_ostruct_recursively` to turn it into a "deep"
openstruct that can be passed to the `WorldwideOrganisation` initializer.

I was using the Hashugar Gem to create the "deep" openstruct before @floehopper
pointed out the code in `GdsApi::Response`.

I experimented with _always_ loading the FCO organisation data in
`stub_world_location` but this resulted in the non-regression tests taking an
additional couple of minutes to run on my machine.
chrisroos added a commit that referenced this pull request Jul 5, 2016
Trello card: https://trello.com/c/Hiizo5g6

This follows on from the work in PRs #2540 and #2555 and applies the same
changes to the regression tests.

Switching from WebMock to Mocha appears to reduce the time taken to run all
regression tests by about 15 minutes on my machine.

== Regression test duration before this commit:

    $ time RUN_REGRESSION_TESTS=true \
    ruby -Itest test/regression/smart_answers_regression_test.rb

    real  56m34.146s
    user  45m49.017s
    sys   9m19.234s

== Regression test duration after this commit:

    $ time RUN_REGRESSION_TESTS=true \
    ruby -Itest test/regression/smart_answers_regression_test.rb

    real  36m29.928s
    user  27m45.692s
    sys   8m33.085s

== Notes

The main difference between this and PRs #2540 and #2555 is that a number of
the regression tests rely on `WorldLocation#fco_organisation` returning a
`WorldwideOrganisation` rather than nil. I've satisfied this requirement by
adding the optional `load_fco_organisation_data` parameter to both
`#stub_world_locations` and `#stub_world_location`, and loading the FCO data
when the parameter is true.

The implementation in `#stub_world_location` uses the organisation data that
was previously being used in `#setup_worldwide_locations`. I've reimplemented
the logic from `WorldwideOrganisation#fco_sponsored?` by querying the
`organisations_data` hash. On finding an FCO organisation in the data I use
`GdsApi::Response.build_ostruct_recursively` to turn it into a "deep"
openstruct that can be passed to the `WorldwideOrganisation` initializer.

I was using the Hashugar Gem to create the "deep" openstruct before @floehopper
pointed out the code in `GdsApi::Response`.

I experimented with _always_ loading the FCO organisation data in
`stub_world_location` but this resulted in the non-regression tests taking an
additional couple of minutes to run on my machine.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.