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

Get Coverage to 100% #3

Merged
merged 1 commit into from Oct 3, 2017

Conversation

@kinson
Copy link
Contributor

commented Sep 29, 2017

Addressing #2

@coveralls

This comment has been minimized.

Copy link

commented Sep 29, 2017

Coverage Status

Changes Unknown when pulling 2f9b20c on kinson:master into ** on simonewebdesign:master**.

@kinson

This comment has been minimized.

Copy link
Contributor Author

commented Sep 29, 2017

This is still in progress obviously. I have a couple of questions so far.

  1. Each of your doctests so far include pid = self() but not PubSub.start_link. How is it that I need to start the gen server with start link in the tests I wrote to be able to call a function on them?
  • update: I realized that these tests only test the functions they are above and do not actually reach the handle_call/handle_cast functions. This works fine for the handle_cast functions but with the handle call functions I am currently unable to figure out how to test without starting the Genserver.
  1. I will keep searching for this, but I am unsure how I can test the subscribers call on the genserver. I am unable to match against the pid variable thus far and want to be more explicit that an arbitrary array containing one item.

Thanks for your patience and guidance on this, I am sure you could probably do this yourself in less time than it takes to explain it to me!

@coveralls

This comment has been minimized.

Copy link

commented Sep 29, 2017

Coverage Status

Changes Unknown when pulling 39c538b on kinson:master into ** on simonewebdesign:master**.

@coveralls

This comment has been minimized.

Copy link

commented Sep 29, 2017

Coverage Status

Changes Unknown when pulling b37aec8 on kinson:master into ** on simonewebdesign:master**.

@coveralls

This comment has been minimized.

Copy link

commented Sep 30, 2017

Coverage Status

Changes Unknown when pulling 877325e on kinson:master into ** on simonewebdesign:master**.

@coveralls

This comment has been minimized.

Copy link

commented Sep 30, 2017

Coverage Status

Changes Unknown when pulling 5bbdb3c on kinson:master into ** on simonewebdesign:master**.

@simonewebdesign

This comment has been minimized.

Copy link
Owner

commented Oct 2, 2017

Hey @kinson, thanks for this PR! Sorry I haven't found the time to review it yet — will do it soon!

@kinson

This comment has been minimized.

Copy link
Contributor Author

commented Oct 2, 2017

Hey @simonewebdesign no worries at all! There are definitely some places that I want to polish before this gets merged, I'll go ahead and make a few comments on the specific code.

iex> pid = self()
iex> PubSub.subscribe(pid, :my_topic)
iex> PubSub.subscribers(:my_topic) |> Enum.count
1

This comment has been minimized.

Copy link
@kinson

kinson Oct 2, 2017

Author Contributor

this isn't very explicit. In reality PubSub.subscribers returns [#PID<123.1>] or something like that but I am unsure of how to match against the pid type in an array.

This comment has been minimized.

Copy link
@simonewebdesign

simonewebdesign Oct 2, 2017

Owner

Enum.count is good enough, but this would be better:

iex> pid = self()
iex> PubSub.subscribe(pid, :my_topic)
iex> [subscriber] = PubSub.subscribers(:my_topic)
iex> subscriber == pid
true
assert Process.exit(pid, :kill) == true
# shouldn't all subscribers be deleted, making this 0?
# IO.inspect PubSub.subscribers(:my_topic)
assert PubSub.subscribers(:my_topic) |> Enum.count == 1

This comment has been minimized.

Copy link
@kinson

kinson Oct 2, 2017

Author Contributor

I am unsure why the subscribers list still includes the killed off pid here

pid = spawn(fn ->
receive do
message ->
assert message == "Hello!"

This comment has been minimized.

Copy link
@kinson

kinson Oct 2, 2017

Author Contributor

I don't believe this is being reached, unsure of how to assert that this subscriber received the message

This comment has been minimized.

Copy link
@simonewebdesign
@coveralls

This comment has been minimized.

Copy link

commented Oct 2, 2017

Coverage Status

Changes Unknown when pulling 2c6f8f3 on kinson:master into ** on simonewebdesign:master**.

Copy link
Owner

left a comment

Added a few minor comments, but looks great! Well done!

iex> pid = self()
iex> PubSub.subscribe(pid, :my_topic)
iex> PubSub.subscribers(:my_topic) |> Enum.count
1

This comment has been minimized.

Copy link
@simonewebdesign

simonewebdesign Oct 2, 2017

Owner

Enum.count is good enough, but this would be better:

iex> pid = self()
iex> PubSub.subscribe(pid, :my_topic)
iex> [subscriber] = PubSub.subscribers(:my_topic)
iex> subscriber == pid
true
mix.lock Outdated
"mimerl": {:hex, :mimerl, "1.0.2", "993f9b0e084083405ed8252b99460c4f0563e41729ab42d9074fd5e52439be88", [:rebar3], []},
"poison": {:hex, :poison, "3.1.0", "d9eb636610e096f86f25d9a46f35a9facac35609a7591b3be3326e99a0484665", [:mix], []},
"ssl_verify_fun": {:hex, :ssl_verify_fun, "1.1.1", "28a4d65b7f59893bc2c7de786dec1e1555bd742d336043fe644ae956c3497fbe", [:make, :rebar], []},
"unicode_util_compat": {:hex, :unicode_util_compat, "0.2.0", "dbbccf6781821b1c0701845eaf966c9b6d83d7c3bfc65ca2b78b88b8678bfa35", [:rebar3], []}}

This comment has been minimized.

Copy link
@simonewebdesign

simonewebdesign Oct 2, 2017

Owner

Could you unstage this file please?


assert Process.exit(pid, :kill) == true
ref = Process.monitor(pid)
assert_receive {:DOWN, ^ref, _, _, _}

This comment has been minimized.

Copy link
@simonewebdesign

simonewebdesign Oct 2, 2017

Owner

Can probably use assert_down here as well?

pid = spawn(fn ->
receive do
message ->
assert message == "Hello!"

This comment has been minimized.

Copy link
@simonewebdesign
@coveralls

This comment has been minimized.

Copy link

commented Oct 2, 2017

Coverage Status

Changes Unknown when pulling a554746 on kinson:master into ** on simonewebdesign:master**.

@coveralls

This comment has been minimized.

Copy link

commented Oct 2, 2017

Coverage Status

Changes Unknown when pulling 24a6689 on kinson:master into ** on simonewebdesign:master**.

@kinson

This comment has been minimized.

Copy link
Contributor Author

commented Oct 2, 2017

Still having issues with assert_receive for the test "process can publish a message to a topic" test. It consistently tells me that the mailbox is empty even though I can see that the receive loop is processing the message. Perhaps the receive loop is processing the message it before the assertion can find it?

@kinson

This comment has been minimized.

Copy link
Contributor Author

commented Oct 2, 2017

Alright I was able to successfully use assert_received with the current process, not a new one. I am not sure if this is comprehensive enough though

@coveralls

This comment has been minimized.

Copy link

commented Oct 2, 2017

Coverage Status

Changes Unknown when pulling 13cb163 on kinson:master into ** on simonewebdesign:master**.

@simonewebdesign

This comment has been minimized.

Copy link
Owner

commented Oct 3, 2017

It's more than enough, thanks! Just squash all the commits in one and I'll merge your PR 👍

@kinson kinson force-pushed the kinson:master branch from 13cb163 to 81adcd6 Oct 3, 2017
@kinson

This comment has been minimized.

Copy link
Contributor Author

commented Oct 3, 2017

Everything should be squashed! 👍

@coveralls

This comment has been minimized.

Copy link

commented Oct 3, 2017

Coverage Status

Changes Unknown when pulling 81adcd6 on kinson:master into ** on simonewebdesign:master**.

@simonewebdesign simonewebdesign merged commit 5cdb8d4 into simonewebdesign:master Oct 3, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on master at 100.0%
Details
@kinson

This comment has been minimized.

Copy link
Contributor Author

commented Oct 3, 2017

Thanks again for your guidance on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.