Skip to content

Smoke tests#42

Merged
mmacai merged 12 commits intomasterfrom
smoke-tests
Feb 27, 2018
Merged

Smoke tests#42
mmacai merged 12 commits intomasterfrom
smoke-tests

Conversation

@mmacai
Copy link
Copy Markdown
Collaborator

@mmacai mmacai commented Feb 14, 2018

  • added Kafka consumer ready check utility function
  • added smoke tests setup (with docker-compose & Elixir ExUnit)
  • added smoke tests cases - proxy, kafka messaging, kafka ready check
  • updated dependencies - had some weird problems with versions while running app/tests
  • removed .dockerignore - unused => we are selectively picking files in Dockerfile anyway
  • updated contribution guide

Test:

  • mix test should work as before
  • see contribution guide for steps how to run smoke tests (they should be sufficient to understand how to run them, if not steps should be updated!)

Closes #33

@mmacai mmacai requested a review from kevinbader February 14, 2018 11:09
@kevinbader
Copy link
Copy Markdown
Contributor

removed .dockerignore - unused => we are selectively picking files in Dockerfile anyway

do you think we should use .dockerignore instead?

@mmacai
Copy link
Copy Markdown
Collaborator Author

mmacai commented Feb 16, 2018

do you think we should use .dockerignore instead?

I read somewhere, that using .dockerignore is best practice and leads to less layers in images, so perhaps we can discuss it and leverage from it in new PR.

end

@tag :smoke
test "not subscribed user shouldn'\t receive message", %{conn: conn} do
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

tab character ... typo?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

ye, that's a typo ... used to do automatic escaping of special characters from JS

@kevinbader
Copy link
Copy Markdown
Contributor

Found one typo.. other than that it looks good to me

Copy link
Copy Markdown
Contributor

@kevinbader kevinbader left a comment

Choose a reason for hiding this comment

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

I get a compile-time warning:

==> rig_outbound_gateway
Compiling 7 files (.ex)
warning: definitions with multiple clauses and default values require a header. Instead of:

    def foo(:first_clause, b \\ :default) do ... end
    def foo(:second_clause, b) do ... end

one should write:

    def foo(a, b \\ :default)
    def foo(:first_clause, b) do ... end
    def foo(:second_clause, b) do ... end

def wait_for_consumer_ready/3 has multiple clauses and defines defaults in one or more clauses
  lib/rig_outbound_gateway/kafka/group_subscriber.ex:123

@kevinbader
Copy link
Copy Markdown
Contributor

Instructions worked like charm, tests seem to work.


@spec wait_for_consumer_ready(String.t(), integer, integer) :: {:ok, String.t()}
def wait_for_consumer_ready(topic, partition, timeout \\ 10_000) do
def wait_for_consumer_ready(topic, partition, timeout) do
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

just a minor comment about the two specs: having one spec per function clause is optional and makes perfect sense if there something like "if I put a String.t in there, I get a String.t out, but if I put an atom in there, it'll be an atom" (you get the idea). However, in this case I feel it would be clearer (and less to write/read/maintain) to have just one combined spec, as I don't see any gain in expressing the same thing using two lines:

@spec wait_for_consumer_ready(String.t(), integer, integer) :: {:ok | :error, String.t()}

What do you think?

PS: I think integer here should actually be non_neg_integer() in both cases

@mmacai mmacai merged commit 007faf0 into master Feb 27, 2018
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