[match] git storage: allow simultaneous usage of clone_branch_directly and shallow_clone #21716
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nekrich
changed the title
lacostej
reviewed
Dec 12, 2023
| @@ -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.
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)
endThere was a problem hiding this comment.
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.
lacostej
approved these changes
Dec 12, 2023
rogerluan
requested changes
Dec 13, 2023
| ) | ||
| storage.download | ||
| describe "when no branch is specified" do | ||
| it("checkouts the master branch ") do |
There was a problem hiding this comment.
Nit: Extra space, and it's generally a practice to avoid the parenthesis when declaring e.g. describe, it, etc.:
Suggested change
| 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?
SubhrajyotiSen
pushed a commit
to KeepTruckin/fastlane
that referenced
this pull request
Jan 17, 2024
…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>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Checklist
bundle exec rspecfrom the root directory to see all new and existing tests passbundle exec rubocop -ato ensure the code style is validci/circlecibuilds 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_cloneorclone_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.