Conversation
|
Test failures may be not related to this commit. :) |
|
/rebuild |
|
Okay, @j143-bot, I started to rebuild this pull request as build #2175. |
ethomson
left a comment
There was a problem hiding this comment.
Actually, the tests were failing on this commit: there's a memory leak introduced. The summary only shows the first 10 lines or so of stderr, but if you click to open the full results, you'll be able to see the valgrind output.
There are a functional changes that I'd recommend in the test itself, but I'm happy to see you adding these tests.
Thanks for the pull request, and welcome to the libgit2 project.
|
|
||
| git_commit_free(other); | ||
|
|
||
| cl_git_pass(git_commit_nth_gen_ancestor(&other, commit, 3)); |
There was a problem hiding this comment.
You need to git_commit_free(other) at the end of this function.
| git_oid_fromstr(&oid, "e90810b8df3e80c413d903f631643c716887138d"); | ||
| git_oid_fromstr(&oid2, "be3563ae3f795b2b4353bcce3a527ad0a4f7f644"); | ||
|
|
||
| cl_assert_equal_i(0, git_graph_ahead_behind(&ahead, &behind, _repo, &oid, &oid2)); |
There was a problem hiding this comment.
These should be cl_git_pass(...) instead of cl_assert_equal_i(0, ...), they'll emit helpful error messages if anything failed.
| cl_git_pass(git_commit_nth_gen_ancestor(&other, commit, 3)); | ||
|
|
||
| cl_assert_equal_i(0, git_graph_ahead_behind(&ahead, &behind, _repo, git_commit_id(commit), git_commit_id(other))); | ||
| cl_assert_equal_i(0, git_graph_ahead_behind(&ahead, &behind, _repo, git_commit_id(other), git_commit_id(commit))); |
There was a problem hiding this comment.
You have several tests that ensure that git_graph_ahead_behind works, but you're not actually examining the results of ahead and behind to ensure that we're returning the correct results.
You should be able to look at the test resource directory in question (I tend to copy it to a temp folder, then rename .gitted to .git. Then run the ahead/behind counts with git itself, and use that to test ahead and behind. (eg, cl_assert_equal_sz(42, ahead)...
|
Repo for testing. |
|
Hi @ethomson - can this reviewed. Thanks. |
pks-t
left a comment
There was a problem hiding this comment.
Sorry for taking so long to review. Only minor nits -- will merge if those were addressed :)
| git_oid oid; | ||
| cl_git_pass(git_repository_open(&_repo, cl_fixture("testrepo.git"))); | ||
|
|
||
| git_oid_fromstr(&oid, "be3563ae3f795b2b4353bcce3a527ad0a4f7f644"); |
|
|
||
| void test_graph_ahead_behind__returns_correct_result(void) | ||
| { | ||
|
|
There was a problem hiding this comment.
Style nit: the empty line here is not required
| git_commit *other; | ||
|
|
||
| git_oid_fromstr(&oid, "e90810b8df3e80c413d903f631643c716887138d"); | ||
| git_oid_fromstr(&oid2, "be3563ae3f795b2b4353bcce3a527ad0a4f7f644"); |
There was a problem hiding this comment.
These two should be wrapped in cl_git_pass
| cl_assert_equal_sz(ahead + 4, behind); | ||
|
|
||
| git_commit_free(other); | ||
|
|
There was a problem hiding this comment.
This newline isn't required, either
| _repo = NULL; | ||
| } | ||
|
|
||
| void test_graph_ahead_behind__returns_correct_result(void) |
There was a problem hiding this comment.
Style: the whole function is indented with spaces instead of with tabs
| static size_t ahead; | ||
| static size_t behind; | ||
|
|
||
| void test_graph_ahead_behind__initialize(void) |
There was a problem hiding this comment.
This one is indented with spaces, too
|
Tried to keep code in 4 spaces = 1 tab style. Let me know, if there are other comments. :) |
pks-t
left a comment
There was a problem hiding this comment.
Looks good to me now, always very happy to merge new tests! Thanks a lot :)
This
ahead_behind, inspired fromdescendant_of. I would like to contribute to this project, with your help along the way!