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
[match] git storage: allow simultaneous usage of clone_branch_directly and shallow_clone #21716
[match] git storage: allow simultaneous usage of clone_branch_directly and shallow_clone #21716
Conversation
@@ -43,6 +43,46 @@ | |||
expect(File.exist?(File.join(storage.working_directory, 'README.md'))).to eq(false) # because the README is being added when committing the changes | |||
end | |||
|
|||
it "directly clones a single branch wtih shallow clone" do | |||
# GIVEN |
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.
what about being a bit more DRY and move those initializations in one place, either using a before
or an around
under the same describe
? I mention around in case the garbage collector kicks in.
describe "#download" do
before(:each) do
@path = Dir.mktmpdir # to have access to the actual path
allow(Dir).to receive(:mktmpdir).and_return(@path)
end
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.
Refactored tests in 85c8ace.
Combined your and @rogerluan comments.
Git spec helper:
- branch checkout commands were repeated all over the spec.
- Command execution check is too long, imo. Added a handful wrapper that accepts a single command string or array of commands.
Spec:
- Default git url and branch.
- Default temp dir stub.
- Shorter test cases.
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.
Looks good.
A potential improvement in or test setup noted.
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 for another contribution @nekrich ! 🙏
Just left a minor comment, let me know what you think 😊
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.
The new refactor looks great!! Just requesting a minor style change and this looks good to go 🤗
) | ||
storage.download | ||
describe "when no branch is specified" do | ||
it("checkouts the master branch ") do |
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.
Nit: Extra space, and it's generally a practice to avoid the parenthesis when declaring e.g. describe
, it
, etc.:
it("checkouts the master branch ") do | |
it "checkouts the master branch" do |
This should've been caught by rubocop but we're not enforcing this particular style yet 😅 could you change it throughout this file?
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.
🚀
…y and shallow_clone (fastlane#21716) * [match] combine clone_branch_directly and shallow_clone * fix: typo Co-authored-by: Roger Oba <rogerluan.oba@gmail.com> * chore: refactor git_storage tests * Remove parenthesis around "it" statements. --------- Co-authored-by: Roger Oba <rogerluan.oba@gmail.com>
Checklist
bundle exec rspec
from the root directory to see all new and existing tests passbundle exec rubocop -a
to ensure the code style is validci/circleci
builds in the "All checks have passed" section of my PR (connect CircleCI to GitHub if not)Motivation and Context
I noticed a slow checkout of our match repo 😅.
How it worked before
You have to choose
shallow_clone
orclone_branch_directly
. The first option clones all branches with depth 1, and another clones branch with full history.Specifying both options (which I've done) led to the shallow cloning of all branches because shallow_clone option took precedence.
How it works in this PR
Description
It was one if/elsif statement. I split it into to independent ones, allowing simultaneous usage of both options.
Tests are provided.
Testing Steps
Run match specifying both options.