Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
5120 – Create Tags in the background #2005
5120 – Create Tags in the background #2005
Changes from 24 commits
97d2cd3
5ab4d0c
7bc2a5f
ee8d6a3
88661be
370d300
dedd1ae
61f48d0
e0e5873
9a2986d
81e95ee
ef52d2f
6781a9c
ffd4a14
3a67d18
c8a0dd2
8a6efa5
93963ab
e35c643
324ad39
b510a7b
935f44f
2c9c1d0
ff01619
5c1c941
9f1141e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vasconsaurus the tests you added in your last commit are still not covering line 15 - I found out that the problem is here...
options
is always a hash, soif options
is alwaystrue
... you should replace this conditional by something likeunless options.blank?
, because when there are no parameters,options
value is{}
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@caiosba I updated this, but have a question regarding tests:
The two tests I added were passing before, so they are not really working as expected. How could I fix it?
Also, is there any extra logging or error handling that I should add?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, Manu. No, I don't think any additional logging or error handling is needed here. But for the test, I suggest you actually assess that the method is executed. For now your test for the no-arguments case just verifies that no exception is raised.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true 🤦♀️ still, do you think I should verify, assert anything else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the test I think you should make sure that the method is executed... for example,
Blank.create!
is a class method that doesn't take any arguments and you can useassert_difference 'Blank.count' do
to make sure it was executed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated to use
assert_difference
both here and for the test in line 28.But one last comment: even when the test wasn't hitting the
else
, because I was usingif options
, the test still succeeded. So blank was still created.So I guess what I mean is, this test makes sure
Blank
is created, but it doesn't make sure of which branch we are hitting.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, I think that the other block was still able to handle it just fine. That said, maybe the if/else is not even needed, but it's more readable/clear to have them separate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, so I was wondering about that. I think it might be nice to have the option with just the one straightforward step, but it isn't really needed. Should we keep it or remove it (the conditional)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's what I said, I think it's more clear to have the conditional. Ruby Meta programming can be harder to read, otherwise :)