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

Allow checking the entire ProducerMessage in the mock producers #1956

Merged
merged 1 commit into from
Jun 9, 2021

Conversation

joewreschnig
Copy link
Contributor

This is done by introducing MessageChecker, a function which validates
an entire ProducerMessage, and reimplementing the existing expectation
interface on top of that. Since producerExpectation was not a public
API, the only visible effect of this to users of the mock producers
should be slightly different error messages when encoding fails.

To better mimic the real producers’ behavior, this also runs
partitioning before passing the message to the checker. By default all
topics have 32 partitions but this can be configured per-topic in either
producer via the embedded TopicConfig. (This does not allow mocking
unavailable partitions; I don’t see a use case for this which can’t be
better handled by unit testing the partitioner.) This may affect
existing tests if someone was passing a configuration with a
Partitioner that would fail on their test messages, but it seems more
likely they want to know if this is the case (we did).

This is useful for (at least) two reasons,

  • I would often like to test more than just the message value; services
    may need to produce to different topics or with specific partitioning
    schemes. In some cases valuable data may only be in the message key,
    or I want to test that a header and message body agree.

  • Since the topic usually also corresponds to the data format, exposing
    the topic makes it easier to collect messages of different types
    across different topics which may appear in known quantities but
    non-deterministic orders. Without the topic this is only possible by
    trying to content-sniff the format from the value, which leads to
    weaker and more fragile tests.

This is done by introducing `MessageChecker`, a function which validates
an entire `ProducerMessage`, and reimplementing the existing expectation
interface on top of that. Since `producerExpectation` was not a public
API, the only visible effect of this to users of the mock producers
should be slightly different error messages when encoding fails.

To better mimic the real producers’ behavior, this also runs
partitioning before passing the message to the checker. By default all
topics have 32 partitions but this can be configured per-topic in either
producer via the embedded `TopicConfig`. (This does not allow mocking
unavailable partitions; I don’t see a use case for this which can’t be
better handled by unit testing the partitioner.) This may affect
existing tests if someone was passing a configuration with a
`Partitioner` that would fail on their test messages, but it seems more
likely they want to know if this is the case (we did).

This is useful for (at least) two reasons,

- I would often like to test more than just the message value; services
  may need to produce to different topics or with specific partitioning
  schemes. In some cases valuable data may only be in the message key,
  or I want to test that a header and message body agree.

- Since the topic usually also corresponds to the data format, exposing
  the topic makes it easier to collect messages of different types
  across different topics which may appear in known quantities but
  non-deterministic orders. Without the topic this is only possible by
  trying to content-sniff the format from the value, which leads to
  weaker and more fragile tests.
@joewreschnig joewreschnig requested a review from bai as a code owner June 1, 2021 18:44
@ghost ghost added the cla-needed label Jun 1, 2021
@joewreschnig
Copy link
Contributor Author

After filling out the CLA it asked me to re-run the CI check but don't see how to do that. I don't think I can until I'm approved to run workflows.

@ghost ghost removed the cla-needed label Jun 2, 2021
@joewreschnig
Copy link
Contributor Author

Sorry, I can't see anything actually broken in the CI, nor a way to trigger a retry myself. Given exit code 28 in the 1.16/2.7.1 case it seems like curl might have timed out trying to install golangci-lint. I ran v1.37.1 (and various other versions) locally with 1.16 and saw no errors.

Copy link
Collaborator

@dnwe dnwe left a comment

Choose a reason for hiding this comment

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

Thanks for the first-time contribution @joewreschnig !

These changes look good to me — thank you for the detailed description

@dnwe dnwe merged commit 03b4a4f into IBM:master Jun 9, 2021
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