Refactor NameID format related tests #10727
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This is mostly salvaged from the work that was merged (and reverted) for the NameID format validation change - 30cb0f3 |
Sgtpluck
approved these changes
May 31, 2024
|
My rationale was that it makes tests hard to maintain when they assert too much. There are other tests that assert that the event is emitted. When we take up the logging of the requested NameID format story, we would add some assertions for the event back. With that said, the refactoring into shared examples does make it easier to maintain the tests even with asserting the analytics events. |
Comment on lines
-1743
to
-1738
| expect(@analytics).to have_received(:track_event). | ||
| with('SAML Auth', analytics_hash) |
There was a problem hiding this comment.
I agree with the previous comment that we should keep some of the assertions around analytics logging in these specs.
I agree that asserting an entire payload migth be too much, but we have a pattern of logs of hash_including that let us assert against a subset
expect(@analytics).to have_logged_event('SAML Auth', hash_including(events_subset))I think that's particularly relevant this week, because the deploy we rolled back was due to us logging the wrong information, so if we intend to fix the logging, it would be good to have specs around said logging
vrajmohan
force-pushed
the
refactor-saml-controller-tests
branch
from
8a1851c
to
59e0218
Compare
Sgtpluck
approved these changes
Jun 4, 2024
vrajmohan
force-pushed
the
refactor-saml-controller-tests
branch
from
59e0218
to
e0d4a58
Compare
**Why**: 1. The tests are hard to understand and do not clearly show the different contexts 2. The tests assert on a side effect (the analytics event posted) rather than the actual desired behavior (the return values) 3. The tests are actually misleading - they conflate "unspecified" and unsupported **How**: - Refactor tests with clear contexts and shared examples - Assert on actual return value and not on a side effect - Clean up test case descriptions - Assert that the correct attributes are logged in the analytics event changelog: Internal, Automated Testing, Refactor NameID format related tests
vrajmohan
force-pushed
the
refactor-saml-controller-tests
branch
from
e0d4a58
to
e508fee
Compare
Merged
aduth
added a commit
that referenced
this pull request
Jun 6, 2024
* Bump phonelib from 0.8.8 to 0.8.9 (#10746) Bumps [phonelib](https://github.com/daddyz/phonelib) from 0.8.8 to 0.8.9. - [Release notes](https://github.com/daddyz/phonelib/releases) - [Changelog](https://github.com/daddyz/phonelib/blob/master/CHANGELOG.md) - [Commits](daddyz/phonelib@v0.8.8...v0.8.9) --- updated-dependencies: - dependency-name: phonelib dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Don't add SP costs twice if worker already did it (#10743) Update VerifyInfoConcern to not add SP costs. changelog: Internal, Identity verification, Guard against double-counting SP costs * Revert "Update to newest saml_idp (#10734)" (#10748) This reverts commit 21bb1cf. * Update Spanish, and Chinese content that was missed in big translation (#10719) * Update Spanish content that was missed in big translation changelog: Internal, Internationalization, Update Spanish content * add additional Chinese translations * Normalize FormResponse documentation in AnalyticsEvents (#10745) * Document standard FormResponse analytics consistently changelog: Internal, Analytics, Document standard FormResponse analytics consistently * Restore optional errors passed in webauthn_setup_submitted * Restore multi_factor_auth_setup errors as nilable * Try disregarding nil values from actual logged These aren't really logged as meaningfully query values anyways, and we should only care if we explicitly expect a nil value * Update expect receive to have_logged_event * Remove unused error_details from piv_cac_login * Add missing error_details * Remove unnecessary allowed_extra_analytics * Revert unused error_details from webauthn_setup_submitted * Add missing error_details in spec assertions * Remove unused error_details from idv_in_person_locations_searched * Remove invalid error_details Remove invalid errors property from idv_in_person_locations_searched * Restore idv_in_person_locations_searched errors as nil-able Sometimes it's sent * Remove error_details from vendor submitted event Not passed (via DocAuthClient response) Remove unused analytics YARDoc error_details * Update IdV analytics spec expectations * Revert unused error_details in fraud review events * Simplify diff * Expect success, errors always in logout events * Make success, errors nilable for logout_initiated Not always provided: https://github.com/18F/identity-idp/blob/a10bd0b98d30d64d08eac23df91db3aa0830e363/app/controllers/sign_out_controller.rb#L7 * Remove unnecessary allowed_extra_analytics in accessibility specs (#10749) * Remove unnecessary allowed_extra_analytics in accessibility specs changeog: Internal, Automated Testing, Remove unnecessary allowed_extra_analytics in accessibility specs * Add changelog changelog: Internal, Automated Testing, Remove unnecessary allowed_extra_analytics in accessibility specs * LG-13221 New device sign-in delayed authentication doesn't list failed MFA attempts (#10659) * changelog: Bug Fixes, Fraud prevention, new device sign in list failed mfa attempts * adjust how created_at is calculated * change update sign_in_new_device_at from last disavowal to last notification expir time * fix specs that were missing the expected timeframe_expired event * add guardrails against missing timeframe_expired and limit query for performance * reduce delay buffer because of stale events, reset test from before this branch * refactor extraneous fetching in query add analytics for missing timeframe_expired event * replace mis-deleted block * revise timeframe expir query, update spec to include timeframe_expired paths, restore previous test setups * account for actual ordering of events * move timeframe_expired_event to private method, add regression test to new device spec * rename method for clarity of purpose * LG-13352 fixed bug where new_unique_users_year_unknown columns were not populating (#10752) * Fixed the bug where the new_unique_users_unknown column was not populating properly changelog: Internal, Reporting, Fixed the bug where the new_unique_users_unknown column was not populating properly * Refactor NameID format related tests (#10727) **Why**: 1. The tests are hard to understand and do not clearly show the different contexts 2. The tests assert on a side effect (the analytics event posted) rather than the actual desired behavior (the return values) 3. The tests are actually misleading - they conflate "unspecified" and unsupported **How**: - Refactor tests with clear contexts and shared examples - Assert on actual return value and not on a side effect - Clean up test case descriptions - Assert that the correct attributes are logged in the analytics event changelog: Internal, Automated Testing, Refactor NameID format related tests * LG-13383 Use the SP issuer to compute the UUID and UUID prefix in the `ResolutionProofingJob` (#10728) In #10726 started passing the service provider issuer to the `ResolutionProofingJob`. This commit uses it to compute the UUID and UUID prefix for downstream proofers. This will eventually supersede the logic that does the same in the `VerifyInfoConcern`. The logic is left in the `VerifyInfoConcern` since we will still need to pass the UUID and UUID prefix to jobs until this is fully deployed. Once this is fully deployed that logic can be removed. [skip changelog] * Remove `instant_verify_ab_test_discriminator` background job argument (#10725) The code that depended on this argument was removed in #10724. The argument was left to avoid `ArgumentError`s during the 50/50 state. This commit removes the argument. It cannot be deployed until the changes in #10724 are fully deployed in production. [skip changelog] * Validate unnecessary exempted files in TypeScript enforcement (#10760) * Validate unnecessary exempted files in TypeScript enforcement changelog: Internal, Automated Testing, Validate unnecessary exempted files in TypeScript enforcement * Remove unnecessary array splat * Create production-ready docker image (#10738) Add prod idp image generation **Why**: - We need to have a production-like IDP docker image so we can start testing idp environments in kubernetes. **How**: - Created a dockerfile that is more production-ready than our review-apps one. - Added real .mmdb files and pwned_passwords.txt files - Added RDS cert - Changed file permissions so the app user cannot change the code - Moved the application.yml file from tmp to config - Created an application.yml file for the production dockerfile in case we want to put defaults in it. - Added a section to .gitlab-ci.yml that should build the idp image and put it in ECR. changelog: Internal, Platform Automation, Add production IDP image creation * Log the requested NameID format (#10761) changelog: User-Facing Improvements, Analytics, Log the requested NameID format * Bump libphonenumber-js from 1.11.2 to 1.11.3 (#10766) Bumps [libphonenumber-js](https://gitlab.com/catamphetamine/libphonenumber-js) from 1.11.2 to 1.11.3. - [Changelog](https://gitlab.com/catamphetamine/libphonenumber-js/blob/master/CHANGELOG.md) - [Commits](https://gitlab.com/catamphetamine/libphonenumber-js/compare/v1.11.2...v1.11.3) --- updated-dependencies: - dependency-name: libphonenumber-js dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Use ivars to clean up `ProgressiveProofer` methods (#10764) The `ProgressiveProofer` class has a `#proof` method which does the bulk of the footwork when it comes to orchestrating proofing API calls on the Verify-Your-Info step. This includes things like double address verification, get-to-yes, and skipping necessary API calls for users who cannot pass. Prior to this commit this class did not store any instance variables. This meant that the child methods in the job needed all of the context necessary to do their work passed in as arguments. This led to long lists of keyword arguments for these methods which distracted from the methods' purpose. This commit refactors the `ProgressiveProofer` to take the arguments that were passed to `#proof` in an initializer and store them as ivars. It also makes a change to store the vendor results as ivars. Finally since the PII passed into the job is an ivar it adds helper methods for transforming and operating on the PII with the aim of making that self-documenting. This adds some brevity to the implementation and hopefully removes some of the distraction that was associated with tracing arguments through the call stack. [skip changelog] * Update rails to address CVE-2024-28103 (#10770) changelog: Internal, Dependencies, Update dependencies to latest versions * LG-13503 Billing Model V2 Enhancments (#10769) changelog: Internal, Reporting, Modified logic in partner helper to reflect new billing requirements * LG-13503 Modified logic in partner helper to reflect new billing requirements * updated issuer columns to sync with partner better * updated compound key usage for uniqueness to a more unpackable object * adding spec to combined invoice report to account for single issuer partners matching idv age columns * Fixing date values to use utc --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Matt Hinz <matt.hinz@gsa.gov> Co-authored-by: Davida (she/they) <davida.marion@gsa.gov> Co-authored-by: Mitchell Henke <mitchell.henke@gsa.gov> Co-authored-by: Kevin Masters <135744319+kevinsmaster5@users.noreply.github.com> Co-authored-by: Colter <59977618+colter-nattrass@users.noreply.github.com> Co-authored-by: Vraj Mohan <vraj.mohan@gsa.gov> Co-authored-by: Jonathan Hooper <jonathan.hooper@gsa.gov> Co-authored-by: Tim Spencer <timothy.spencer@gsa.gov> Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why:
How:
changelog: Internal, Automated Testing, Refactor NameID format related tests