Skip to content
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.

Sign up for GitHub

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

Jump to bottom

LibGit2: tests & improvements #13417

Merged
merged 3 commits into from
Oct 7, 2015
Merged

LibGit2: tests & improvements #13417

merged 3 commits into from
Oct 7, 2015

Conversation

wildart
Copy link
Member

@wildart wildart commented Oct 2, 2015

PR provides following:

  • more tests for LibGit2 module (separated online part)
  • AbstractPayload type for various callback payloads
  • Credential callback can handle payload parameter (i.e. credentials)
  • Payload support to high level functions: clone, fetch, push
  • Some additional functions for remote references which are required for tests
  • Function which generates for github credentials from git global config and 2FA token (if available)

@wildart wildart added the packages Package management and loading label Oct 2, 2015
tkelman
tkelman reviewed Oct 2, 2015
View reviewed changes
test/libgit2.jl
@@ -1,9 +1,12 @@
# This file is a part of Julia. License is MIT: http://julialang.org/license

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this first whitespace is considered significant and will get added back in by the license-headers script

@wildart wildart added the test This change adds or pertains to unit tests label Oct 2, 2015
tkelman
tkelman reviewed Oct 2, 2015
View reviewed changes
test/libgit2.jl
repo_path = joinpath(dir, "Example3")
@test_throws LibGit2.Error.GitError LibGit2.clone(ssh_prefix*repo_url, repo_path)
end
catch ex
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a forced failure at the end of the try if it should always fail

@tkelman
Copy link
Contributor

tkelman commented Oct 2, 2015

While you're refactoring the tests, can you move any that require internet access either to pkg or a separate libgit2-online test file? we cant run those by default but can add them to travis and appveyor

@wildart wildart changed the title improved credentials callback improved credentials callback & libgit2 tests Oct 3, 2015
tkelman
tkelman reviewed Oct 3, 2015
View reviewed changes
test/libgit2.jl
finally
remove_tmp_dir && rm(tmpdir, recursive=true)
# test parameters
repo_url = "github.com/JuliaLang/Example.jl"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cloning this requires net access, right? add libgit2-online to the travis and appveyor yml's

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not clone it here, it's used to setup remote reference only.

@wildart wildart changed the title improved credentials callback & libgit2 tests libgit2: tests & improvements Oct 5, 2015
@wildart wildart changed the title libgit2: tests & improvements LibGIt2: tests & improvements Oct 5, 2015
@wildart wildart changed the title LibGIt2: tests & improvements LibGit2: tests & improvements Oct 5, 2015
@wildart
Copy link
Member Author

wildart commented Oct 5, 2015

Ready for review.
cc @tkelman @jakebolewski @kshyatt

@wildart wildart mentioned this pull request Oct 5, 2015
Closed
tkelman
tkelman reviewed Oct 5, 2015
View reviewed changes
base/libgit2.jl
checkout_tree(repo, btree)
end
if set_head
# checout selected branch
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checkout

@wildart
added credentials callback with payload to all high livel network fun…
53c2a2c 
…ctions: clone, fetch, push

credential callback can handle payload
added `fetch_refspecs` & `push_refspecs` funcs for remote reference
added function for github credentials
refined repo initialization tests
fixed branch head switching
added signature parameter for `tag_create`
added checkout options as a parameter
improved libgit2 tests & separated online reqired
provide signature to avoid reading from git global configuration
fixed tests to handle testing system with(out) a default git configuration
fixed typos
@wildart
Copy link
Member Author

wildart commented Oct 6, 2015

Squashed it. I need to merge this to continue work on #13387 and PkgDev

jakebolewski
jakebolewski reviewed Oct 6, 2015
View reviewed changes
base/libgit2/callbacks.jl
if payload_ptr != C_NULL
payload = unsafe_pointer_to_objref(payload_ptr)
if isa(payload, AbstractPayload)
username = isdefined(payload, :user) ? getfield(payload, :user) : ""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel that checking for specific field names for a given type should be wrapped in a generic function: user(::Payload), password(::Payload), isused(::Payload), etc.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea.

@wildart wildart mentioned this pull request Oct 7, 2015
Closed
jakebolewski added a commit that referenced this pull request Oct 7, 2015
@jakebolewski
Merge pull request #13417 from wildart/callbacks
4851ee9 
LibGit2: tests & improvements
@jakebolewski jakebolewski merged commit 4851ee9 into JuliaLang:master Oct 7, 2015
@wildart wildart deleted the callbacks branch October 7, 2015 11:47
@tkelman
Copy link
Contributor

tkelman commented Oct 13, 2015

@testset is kind of noisy, we should shut off the summary results display when everything succeeds

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
packages Package management and loading test This change adds or pertains to unit tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@wildart @tkelman @jakebolewski @ScottPJones