Skip to content

Inline keyword arguments passed to have_logged_event#11012

Merged
aduth merged 1 commit intomainfrom
aduth-inline-have-logged-event-hash
Aug 12, 2024
Merged

Inline keyword arguments passed to have_logged_event#11012
aduth merged 1 commit intomainfrom
aduth-inline-have-logged-event-hash

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Jul 31, 2024

🛠 Summary of changes

Updates analytics assertions in specs to inline properties passed to have_logged_event, rather than creating a separate variable to pass.

This is a somewhat subjective stylistic preference, though some more objective rationale is included below. It's meant to complement ongoing effort to standardize and refine logging assertions in specs.

Why?

  • Improves readability of tests
    • Eyes are not moving around to find references to hash variable
      • Keyword arguments referenced as a variable are hard to keep colocated with the actual assertion, and often drift away randomly
      • Especially when the analytics hash is defined outside the spec itself, e.g. as an RSpec let memoized variable, which is unnecessary as we usually ought to only assert the expected log result once in most cases, and the variable is not reused
    • Clearer separation of "Arrange Act Assert"
    • Fewer curly braces 😉
  • Improves consistency
    • Some tests have mixed implementation of have_logged_event inline vs. referenced variable
  • Encourages best practices
    • Sometimes a variable can be used to "DRY" up repeated log properties, but this should instead be a signal that we're redundantly logging the same thing multiple times and we should address the root problem.
    • Sometimes a variable can be used to avoid excessive indentation when used in combination with hash_including matching assertion, but I'm of the opinion that we should generally always assert exactly what we're expecting, as hash_including can often produce false sense of confidence when properties are added in the future without accountability to corresponding spec expectation changes.
  • Code size
    • See "Additions" and "Removals" summary in changed files ("556 additions and 652 deletions" at time of writing)

Identified using a few search patterns in spec/ (usually filtered to spec/controllers and spec/features):

  • have_logged_event\('.+?[^'):]\)
  • (analytics_hash|properties|attributes|result) = \{

📜 Testing Plan

Verify tests pass.

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.

BIG FAN. LGTM

@aduth
Copy link
Contributor Author

aduth commented Aug 1, 2024

Noting that I based this branch off the one in #10987 to save myself some merge conflict headaches, so will have to wait 'til that one wraps up before merging.

@aduth aduth force-pushed the aduth-analytics-compact-event-properties branch 3 times, most recently from 71b5c98 to d815f0a Compare August 9, 2024 12:08
Base automatically changed from aduth-analytics-compact-event-properties to main August 9, 2024 12:24
@aduth aduth force-pushed the aduth-inline-have-logged-event-hash branch from c7be4cb to a0fde55 Compare August 9, 2024 16:13
@aduth aduth marked this pull request as ready for review August 9, 2024 16:14
changelog: Internal, Code Quality, Improve readability of automated test analytics assertions
@aduth aduth force-pushed the aduth-inline-have-logged-event-hash branch from a0fde55 to 3a2aa57 Compare August 12, 2024 14:48
@aduth aduth merged commit 829700b into main Aug 12, 2024
@aduth aduth deleted the aduth-inline-have-logged-event-hash branch August 12, 2024 15:35
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