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

improve mocking framework for tests #871

Merged
merged 2 commits into from
Jan 16, 2025
Merged

improve mocking framework for tests #871

merged 2 commits into from
Jan 16, 2025

Conversation

panentheos
Copy link
Collaborator

Summary of changes

This makes several improvements related to mocking and test infrastructure.

  • Stores mockable module references in the application environment rather than process state. This reduces the amount of per-process state, and simplifies several code paths.
  • Inlines callback specs in the relevant engine modules, rather than maintaining separate behavior modules.
  • Defines proper mocks for bus-related engines.
  • Enables the capture_log option, eliminating log spam when running tests (logs will still be printed if a test fails).

@panentheos panentheos requested a review from a team as a code owner January 14, 2025 18:57
Copy link
Contributor

@digitalcora digitalcora left a comment

Choose a reason for hiding this comment

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

I like it! 🧹

lib/realtime_signs.ex Outdated Show resolved Hide resolved

Process.sleep(50)
assert Engine.Config.sign_config("chelsea_outbound", :auto) == :off
end
Copy link
Contributor

Choose a reason for hiding this comment

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

What do these removals relate to? Not sure how they tie into the mocking changes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, this leaked through from another change I was testing, but backed out. I was seeing if we could disable the application entirely in test mode to further reduce log spam. There were a handful of tests that failed because they access the actual application processes.

Even though it's unrelated, this still seems worth keeping, because this style of test is unnecessarily brittle for what it's doing. The two deleted cases were duplicated by other tests later in the file, and the third one is rewritten to use the more isolated approach.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha. I'd agree this seems brittle, and if the behavior is already covered by other tests, no problem with removing these.

test/signs/bus_test.exs Outdated Show resolved Hide resolved
Copy link
Contributor

@digitalcora digitalcora left a comment

Choose a reason for hiding this comment

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

@panentheos panentheos merged commit c2dc678 into main Jan 16, 2025
2 checks passed
@panentheos panentheos deleted the bhw/test-improvements branch January 16, 2025 17: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