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

LG-12532 Add 50/50 tests for IPP state id page #11090

Merged
merged 1 commit into from
Aug 19, 2024
Merged

Conversation

shanechesnutt-ft
Copy link
Contributor

🎫 Ticket

LG-12532

🛠 Summary of changes

Added integration tests for covering 50/50 state for the state ID page FSM and Controller.
Added integration tests for the state ID page controller.

@@ -36,7 +36,7 @@
<%= link_to(
MarketingSite.help_center_article_url(
category: 'verify-your-identity',
article: 'accepted-state-issued-identification',
article: 'accepted-identification-documents',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want to point out that this change was made due to the integration tests I added for state_id_controller.

I was getting an ActionView::Template::Error:Unknown help center article category verify-your-identity/accepted-state-issued-identification, so I updated it to the accepted-identification-documents article that was defined in the MarketingSite service.

@shanechesnutt-ft shanechesnutt-ft marked this pull request as ready for review August 15, 2024 17:54
@shanechesnutt-ft shanechesnutt-ft requested review from a team and jennyverdeyen August 15, 2024 18:22
@@ -0,0 +1,378 @@
require 'rails_helper'

RSpec.describe 'state id 50/50 state', js: true, allowed_extra_analytics: [:*],
Copy link
Contributor

Choose a reason for hiding this comment

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

Depending if #11099 is merged before this one, we should rebase and check whether allowed_extra_analytics is still necessary. Ideally we'd not be adding new instances of this, as it's meant for legacy only.

Copy link
Contributor Author

@shanechesnutt-ft shanechesnutt-ft Aug 16, 2024

Choose a reason for hiding this comment

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

Okay thanks for the heads up! I can rebase this branch off of the branch for the PR using git rebase --onto and try it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After rebasing with your branch, it appears that the allow_extra_analytics is still needed. When I removed allowed_extra_analytics from the describe block I receive the following error message:

FakeAnalytics::UndocumentedParams:
              event :idv_doc_auth_welcome_visited called with undocumented params [:opted_in_to_in_person_proofing]
              (if these params are for specs only, use :allowed_extra_analytics metadata)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The opted_in_to_in_person_proofing property seems to be coming from the ab_test_analytics_buckets method if that helps.

Copy link
Contributor

Choose a reason for hiding this comment

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

Discussed on Slack, but we can either add the missing analytics event property documentation, or re-add allowed_extra_analytics. Ideally we're documenting all analytics methods, but I think it'd be fair to call it out of scope.

click_send_link
end

perform_in_browser(:mobile) do
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice use of these helpers for the hybrid flow!

expect(page).to have_current_path(idv_in_person_verify_info_url, wait: 10)
click_button t('idv.buttons.change_state_id_label')

# state id page has fields that are pre-populated
Copy link
Contributor

Choose a reason for hiding this comment

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

Helpful comment for readability!

Copy link
Contributor

@jennyverdeyen jennyverdeyen left a comment

Choose a reason for hiding this comment

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

Amazing work!! I read through the tests and ran them, they pass for me locally. Nice and comprehensive test coverage of the IPP flow when the controller variable is toggled on and off. Easy to understand, consistent, and descriptive nested contexts! Great usage of the helper methods throughout - I learned a lot reading through this!

LGTM!

@gina-yamada gina-yamada self-requested a review August 19, 2024 16:58
Copy link
Contributor

@gina-yamada gina-yamada left a comment

Choose a reason for hiding this comment

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

I added a few comments. They are not blocking.

changelog: Internal, Automated Testing, Add 50/50 state integration
tests for the state_id step in the ID-IPP/EIPP flow in preperation for
removing FSM code.
@shanechesnutt-ft shanechesnutt-ft merged commit 2f77eb1 into main Aug 19, 2024
2 checks passed
@shanechesnutt-ft shanechesnutt-ft deleted the sc/LG-12532 branch August 19, 2024 17:57
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.

4 participants