Add part, quit and nick events #21
Conversation
|
👍 |
There was a problem hiding this comment.
The broken interpolation syntax also concerns me that the code with the regex substitution in the implementation of on_nick_change is not actually being tested. I think the use of stubbing in the tests might be too aggressive. We'd need to make sure the user look ups would actually succeed with the values they are being passed.
| @@ -7,6 +7,7 @@ | |||
| let(:robot) { double("Lita::Robot", auth: authorization) } | |||
| let(:cinch) { double("Cinch::Bot").as_null_object } | |||
| let(:user) { double("Lita::User", name: "Carl") } | |||
| let(:prefix) { "#[user.name}!#{user.name.downcase}@localhost" } | |||
There was a problem hiding this comment.
The interpolation syntax here is wrong ([ instead of {).
|
|
||
| describe "#on_nick_change" do | ||
| before do | ||
| allow(Lita::User).to receive(:find_by_name).and_return(user) |
There was a problem hiding this comment.
Right, a test should obviously fail in case of an error = )
Any idea how to improve it? Since adding another allow(Lita::User)[...] line didn't give any better results. Can you give conditional allow lines or something?
There was a problem hiding this comment.
You can specify the arguments you expect to be passed to the stubbed method using the with method. rspec-mocks's readme is a very good guide: https://github.com/rspec/rspec-mocks
There was a problem hiding this comment.
allow(Lita::User).to receive(:find_by_name).and_return(from_user, user)is how I do it now. Do you prefer:
allow(Lita::User).to receive(:find_by_name).with(user.name).and_return(user)
allow(Lita::User).to receive(:find_by_name).with(from_user.name).and_return(from_user)There was a problem hiding this comment.
This question makes me realize that "user" and "from_user" are confusing names. What is the difference between each? I'm guessing from_user is the old nickname and user is the new one. Maybe new_user and old_user instead, then?
There was a problem hiding this comment.
I've thought about that but I think having user as the one true actionable user is better. Then it follows the theme of all other triggers and they can be easily reused. Otherwise we'd end up with user, new_user and old_user...
Still want me to change it?
There was a problem hiding this comment.
Maybe just change "from_user" to "old_user" and leave "user" as is. "from" is kinda ambiguous cause it sounds like it could mean "the username the person changed from" or "the user the event is from".
| from_user: from_user, | ||
| user: user, | ||
| ) | ||
| expect(from_user.name).to eq(prefix.gsub(/!.*/, '')) |
There was a problem hiding this comment.
How can I test this without essentially duplicating the code?
There was a problem hiding this comment.
You don't have to duplicate the implementation, you just want to ensure that a specific input produces a specific output. Given that, I think it's okay to use hardcoded strings instead of trying to dynamically create the expected output within the test.
There was a problem hiding this comment.
expect(from_user.name).to eq(from_user.name)looks rather silly so
expect(from_user.name).to eq("Dora")it is then.
Closes #16