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

Update the README testing example to one that works #80

Closed

Conversation

michaelherold
Copy link

The example of how to testing Interactors in the README does not work in
the current version of interactor. This is a huge barrier to entry for
anyone looking to use the library. We should update the README to
indicate how tests should actually be done.

Note: This commit is just intended to add the spec to show that it
fails. I will add a second commit in a moment to show the method that
I use to test my Interactors.

The example of how to testing Interactors in the README does not work in
the current version of `interactor`. This is a huge barrier to entry for
anyone looking to use the library. We should update the README to
indicate how tests should actually be done.

Note: This commit is just intended to add the spec to show that it
fails. I will add a second commit in a moment to show the method that
I use to test my Interactors.
@michaelherold
Copy link
Author

Hopefully this PR is structured in a way that makes sense. I can add another commit that removes spec/readme_spec.rb if that's not something that should stick around. If that is wanted, I'll add a [ci skip] since it would just be a documentation update at that point.

Thanks for making such a fun interactor library. I'm really enjoying it compared to the previous library I used and am liking some of the discussion for the future of the gem.

@posgarou
Copy link
Contributor

I was running into the same difficulty just now, looking at the tests in the README.

I think you can also modify the test in the README to replace calls of interactor.call with interactor.run and still get the expected behavior. (See the code for run here.)

@michaelherold
Copy link
Author

Hmm, fair enough. I liked the #call syntax better, as it feels more like issuing a command.

@philipgiuliani
Copy link

Looks good :) @posgarou the .run should not be used, its an internal method and may change in every update.

}.to change {
context.message
}.from(nil).to be_present
expect(subject.message).to_not be_present
Copy link

Choose a reason for hiding this comment

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

If I'm reading this correctly the original expectation is that the messages goes from nil to being set after the subject has been invoked. The new expectation asserts that message isn't set after the subject has been invoked. That doesn't line up with the name of the test or what it is replacing.

Copy link
Author

Choose a reason for hiding this comment

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

Ah yes, that's a typo. Thank you for pointing it out! If you look at the test at spec/readme_spec.rb:47 I actually have it correct. I'll change that right away.

@laserlemon
Copy link
Collaborator

I merged #84 with an updated readme because it applied cleanly. Thank you for your help!

@laserlemon laserlemon closed this May 5, 2015
@michaelherold michaelherold deleted the out-of-date-readme branch May 5, 2015 20:31
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.

5 participants