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

Document sign-in helper analytics events to unexempt extra analytics #10548

Merged
merged 3 commits into from
May 8, 2024

Conversation

aduth
Copy link
Member

@aduth aduth commented May 2, 2024

🛠 Summary of changes

Updates a few analytics methods to specify and document the full set of expected arguments.

Specifically, this addresses issues stemming from shared helpers, where failing to document these may require additional new allowed_extra_analytics exceptions where ideally we'd want only the current set of exempted specs (i.e. exemptions only shrinking, never growing).

Follow-up to discussion at #10405 (comment)

Temporarily merges to #10405, but will rebase to main after #10405 is merged.

📜 Testing Plan

Verify that the build passes.

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! So glad that this small number analytics events could allow removing this exception from a those tests!

RSpec.feature 'Accessibility on static pages', :js, allowed_extra_analytics: [:*] do
RSpec.feature 'Accessibility on static pages', :js do
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️ thanks for taking the time to do this!

@aduth
Copy link
Member Author

aduth commented May 2, 2024

LGTM! So glad that this small number analytics events could allow removing this exception from a those tests!

It could have been that there have been other changes on main that allowed the removal of the exceptions. I took the rather extreme route of removing all of the exceptions in spec/features, and then re-exempting where there were failures.

It might be interesting to see if we can find a way to fail tests if allowed_extra_analytics is added unnecessarily to a spec group.

Base automatically changed from jd-LG-12757-cancel-confirmation-step to main May 7, 2024 22:03
@aduth aduth force-pushed the aduth-sign-in-analytics-exception branch from 1843786 to dabf512 Compare May 8, 2024 12:10
@aduth aduth merged commit 43201d5 into main May 8, 2024
2 checks passed
@aduth aduth deleted the aduth-sign-in-analytics-exception branch May 8, 2024 13:25
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.

None yet

2 participants