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

GUILD-619: Test helpers don't automatically reset to test config #120

Merged
merged 12 commits into from
Aug 12, 2021

Conversation

angad-singh
Copy link
Member

@angad-singh angad-singh commented May 14, 2021

Description

  • Deimos is not reset back to the default test config before each test now.
  • A function has been added which can be called by clients in tests to manually reset Deimos back to test config.
  • The same function can also be used to override settings while keeping defaults in place.

Fixes https://flippit.atlassian.net/browse/GUILD-619

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Tested by calling this function in existing specs locally.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added a line in the CHANGELOG describing this change, under the UNRELEASED heading
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

README.md Outdated
configure_deimos(consumers: { reraise_errors: false },
producers: { topic_prefix: nil },
db_producer: { compact_topics: %w(my-topic my-topic2) },
logger: Rails.logger)
Copy link
Member

Choose a reason for hiding this comment

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

Nice, I like this a lot! I think there might be some other changes to the README that are necessary - e.g. we should be calling out things like the test producer and how to interact with it rather than assuming that when you include TestHelpers you get it for free.

Copy link
Member Author

Choose a reason for hiding this comment

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

@dorner I tried my best to add more info but there might be some that I missed and/or added incorrect information. Please let me know!

README.md Outdated
@@ -935,6 +936,18 @@ expect(message).to eq({
topic: 'my-topic',
key: 'my-id'
})

## Configuring Deimos to test settings
Copy link
Member

Choose a reason for hiding this comment

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

So this shouldn't be in the code block with everything else because it likely won't be in the test itself. Also, the previous examples in this block will only work if the test producers are being used, so we should call that out and put this explanation before that block.

You can give an example of a spec_helper that does it:

# spec_helper.rb
RSpec.before(:each) do
  configure_deimos
end

Actually, now that I write this, it would be even better if we can allow this method to take a block and reset the config back to what it was. Then you could use it like this:

RSpec.around(:each) do |ex|
  configure_deimos({...}) do
    ex.run
  end

...and we would be able to ensure that config didn't leak past examples, which is something that's bitten me multiple times. You can deep-dup the config and restore it after the block is run.

lib/deimos/test_helpers.rb Show resolved Hide resolved
README.md Outdated

# Similarly you can use the Kafka test helper
around(:each) do |example|
Deimos::TestHelpers.kafka_test
Copy link
Member

Choose a reason for hiding this comment

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

missing !

README.md Show resolved Hide resolved
CHANGELOG.md Outdated
@@ -6,6 +6,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

## UNRELEASED
### Roadmap :car:

- TestHelper does not automatically reset Deimos config before each test. [#120](https://github.com/flipp-oss/deimos/pull/120)
Copy link
Member

Choose a reason for hiding this comment

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

Should call out in bold that this is a breaking change.

lib/deimos/test_helpers.rb Show resolved Hide resolved
README.md Outdated

# Kakfa test helper using schema registry
around(:each) do |example|
Deimos::TestHelpers.kafka_schema_registry_test!
Copy link
Member

Choose a reason for hiding this comment

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

Maybe call this full_integration_test?

README.md Show resolved Hide resolved
@dorner dorner merged commit f77ca0f into master Aug 12, 2021
@dorner dorner deleted the GUILD-619 branch August 12, 2021 20:24
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