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

Fail build on unnecessary allowed_extra_analytics #10571

Merged
merged 20 commits into from
May 10, 2024

Conversation

aduth
Copy link
Member

@aduth aduth commented May 8, 2024

🛠 Summary of changes

Builds upon #9946 to enforce that allowed_extra_analytics RSpec example group metadata exactly matches the undocumented parameters discovered in that group.

This helps facilitate a pathway to gradually move away from allowed_extra_analytics and require that all extra analytics are documented.

Related discussion: #10548 (comment)

📜 Testing Plan

Verify that build fails with demonstrated failures in 25f66ab. Build should pass after those changes are reverted (and after any legitimate issues identified by this linter are resolved).

Comment on lines 204 to 216
expect(allowed_extra_analytics).to(
match_array(all_checked_extra_analytics),
"Unnecessary allowed_extra_analytics method names on example group #{group}: " +
(allowed_extra_analytics - all_checked_extra_analytics).to_s,
)
Copy link
Member Author

Choose a reason for hiding this comment

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

Example failure with changes in 25f66ab:

An error occurred in an `after(:context)` hook.
Failure/Error:
  expect(allowed_extra_analytics).to(
    match_array(all_checked_extra_analytics),
    "Unnecessary allowed_extra_analytics method names on example group #{group}: " +
      (allowed_extra_analytics - all_checked_extra_analytics).to_s,
  )

  Unnecessary allowed_extra_analytics method names on example group RSpec::ExampleGroups::UsersPivCacAuthenticationSetupController: [:foo]

Comment on lines 199 to 209
expect(all_checked_extra_analytics).not_to(
be_blank,
"Unnecessary allowed_extra_analytics on example group #{group}",
)
Copy link
Member Author

Choose a reason for hiding this comment

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

Example failure with changes in 25f66ab:

An error occurred in an `after(:context)` hook.
Failure/Error:
  expect(all_checked_extra_analytics).not_to(
    be_blank,
    "Unnecessary allowed_extra_analytics on example group #{group}",
  )

  Unnecessary allowed_extra_analytics on example group RSpec::ExampleGroups::UsersTotpSetupController

spec/support/fake_analytics.rb Outdated Show resolved Hide resolved
spec/controllers/idv/phone_errors_controller_spec.rb Outdated Show resolved Hide resolved
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, would it be good to add a spec to show what this fails like in the fake_analytics_spec.rb?

https://github.com/18F/identity-idp/blob/main/spec/support/fake_analytics_spec.rb#L555

@aduth
Copy link
Member Author

aduth commented May 9, 2024

would it be good to add a spec to show what this fails like in the fake_analytics_spec.rb?

It would be good, yeah. I spent a little time looking at it and I'm not entirely sure how to approach it, since the error only happens after tests are run. And the current implementation also assumes that allowed_extra_analytics is defined on the top-level example group of a file, where existing specs in fake_analytics_spec.rb test this as a per-example metadata.

Unsure if you might any ideas, otherwise I think it could be okay to leave untested? Since this is spec-specific behavior anyways, and ideally will be short-lived during a transitionary period.

aduth and others added 17 commits May 10, 2024 08:23
changelog: Internal, Automated Testing, Fail build on unnecessary allowed_extra_analytics
Scenario: A test calls an analytics method, but all of its arguments are documented. This will be included in checked_extra_analytics, but not an issue. If it was an issue, it would have failed earlier in the UndocumentedParamsChecker check

Alternatively, we could assign method name into checked_extra_analytics only when it's going to be an issue, but the logical ordering of how allowed_extra_analytics is considered relative to extra_keywords makes this difficult.
Scenario: Metadata assigned on group different than highest ancestor?
Avoid endlessly accumulating groups

`after(:all)` happens after each test file, not after _all_ all
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
@aduth aduth force-pushed the aduth-unnecessary-allowed-extra-analytics branch from 2f5ee92 to 7c0d9cd Compare May 10, 2024 12:23
@aduth aduth merged commit 44a6d6f into main May 10, 2024
2 checks passed
@aduth aduth deleted the aduth-unnecessary-allowed-extra-analytics branch May 10, 2024 13:47
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.

2 participants