Skip to content

Improve IdV verify feature specs#6555

Merged
aduth merged 8 commits intomainfrom
aduth-verify-spec-simplify
Jul 8, 2022
Merged

Improve IdV verify feature specs#6555
aduth merged 8 commits intomainfrom
aduth-verify-spec-simplify

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Jul 7, 2022

Why:

  • Improve performance
    • Avoid duplicate preparation that currently exists in many specs which don't explicit opt-out using the skip_step_completion option. Since that's easy to miss, the workaround here was to remove it and inline the preparation in each spec.
    • Consolidate content specs
    • Remove duplicate specs ("resolution failure" and "SSN toggle")
  • Improve accuracy
  • Improve durability
  • Simplify
    • Always stub ApplicationController#analytics rather than for each individual spec

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment on lines -210 to -214
agent = instance_double(Idv::Agent)
allow(Idv::Agent).to receive(:new).and_return(agent)
allow(agent).to receive(:proof_resolution).and_return(
success: true, errors: {}, context: { stages: [] },
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️ thank you for removing all these stubs on .new

it 'does not perform the state ID check' do
allow(IdentityConfig.store).to receive(:aamva_sp_banlist_issuers).
and_return('["urn:gov:gsa:openidconnect:sp:server"]')
expect_any_instance_of(Idv::Agent).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expect_any_instance_of is not my favorite, but given that these are feature specs we don't have much of a choice 😬 and I'd rather have this than stub .new 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expect_any_instance_of is not my favorite

Curious: Could you elaborate on why you'd avoid it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like as is, this is just syntactic sugar over stubbing .new,

In something like a unit test, I'd be able to inject a specific instance to the class we're testing, so I could test that instance, to prevent confusion or test pollution when like, there should maybe be separate Idv::Agent instances or something.

A little testing aphorism I like is "don't stub what you don't own", so if this was a unit test I'd write it like...

step = VerifyStep.new
allow(step).to receive(:idv_agent).and_return(double('Idv::Agent'))

expect(idv_agent).to receive().....
step.call(....)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see what you mean now, and I see how feature specs in particular may not be particularly well suited for the ideal implementation. Thanks for explaining 👍

@aduth aduth merged commit 77705ba into main Jul 8, 2022
@aduth aduth deleted the aduth-verify-spec-simplify branch July 8, 2022 12:02
@jmdembe jmdembe mentioned this pull request Jul 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants