-
-
Notifications
You must be signed in to change notification settings - Fork 32
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
feat: added upgrade option #222
feat: added upgrade option #222
Conversation
Thank you very much @gpuligundla, what I see looks very promising. Just need to add some tests, include you in the changelog.md, and add the new option to the documentation (it would go here: docs/command-line.md). |
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 Agree with @Tito-Kati I'm lacking some acceptance test and some unit test for this
tests/unit/helpers_test.sh
Outdated
function test_get_latest_tag() { | ||
assert_not_empty "$(helpers::get_latest_tag)" | ||
} |
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.
Calling the actual method will imply I/O which by definition is not a unit test, because you are depending on external things. Therefore, we can mock the network response, and we will test the actual behaviour of the function. Consider the following example:
function test_get_latest_tag() { | |
assert_not_empty "$(helpers::get_latest_tag)" | |
} | |
function test_get_latest_tag() { | |
mock git<<EOF | |
fc9aac40eb8e5ad4483f08d79eb678a3650dcf78 refs/tags/0.1.0 | |
a17e6816669ec8d0f18ed8c6d5564df9fc699bf9 refs/tags/0.10.0 | |
3977be123b0b73cfdf4b4eff46b909f37aa83b3c refs/tags/0.10.1 | |
b546c693198870dd75d1a102b94f4ddad6f4f3ea refs/tags/0.2.0 | |
732ea5e8b16c3c05f0a6977b794ed7098e1839e2 refs/tags/0.3.0 | |
EOF | |
assert_equals "0.10.1" "$(helpers::get_latest_tag)" | |
unset -f git # remove the mock | |
} |
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, changed it. I never worked with the concept of mock. Thanks for introducing it to me.
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.
Awesome! Thanks a lot π―
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.
Thank you for collaborating with us and adding this great feature to our library.
Thank you so much, guys. It is my first notable open-source contribution <3 |
π Description
This PR Address the issue: #143
π Changes
β To-do list
CHANGELOG.md
to reflect the new feature or fix