Adding in a smarter parent-base finding algorithm
Review Request #7985 — Created Feb. 21, 2016 and submitted
Main idea
Given a tree like this one:
A - * - * - * - * origin/master
* - B - * - * origin/feature
- C - D topicwhen we rbt post, it would like to have three things,
'base': A revision to use as the base of the resulting diff.
'tip': A revision to use as the tip of the resulting diff.
'parent_base': (optional) The revision to use as the base of a
parent diff.
Now, parent_base has to be in the remote repository, in other words, a ancestor of a commit of any branch origin/*. We would also like parent_base to be as young an ancestor of tip as possible, given the first constraint.A git-rev-list query can find this in the following manner -
git rev-list $TIP --not --remotes=origin | tail -1
should give the oldest ancestor that is not reachable from a public branch, so it's parent will be the best parent base.
The algorithm has been tested extensively. More specifically, GitClient.parse_revision_spec has been tested on the conditions outlined in the following comment (https://reviews.reviewboard.org/r/6797/#comment20865). Detailed documentation and visualization have been added to the tests to aid understanding. I hope this will help a reader understand and review the tests better.
Specifically, the conditions being tested are -
0) Testing GitClient.parse_revision_spec with target branch off a tracking branch not aligned with the remote.
1) Testing GitClient.parse_revision_spec with target branch off a tracking branch aligned with the remote.
2) Testing GitClient.parse_revision_spec with target branch off a tracking branch with changes since the remote.
3) Testing GitClient.parse_revision_spec with target branch off a branch not properly tracking the remote.
4) Testing GitClient.parse_revision_spec with a target branch that merged a tracking branch off another tracking branch.
5) Testing GitClient.parse_revision_spec with a target branch posted off a tracking branch that merged another tracking branch.
6) Testing GitClient.parse_revision_spec with a target branch posted off a remote branch without any tracking branches.
7) Testing GitClient.parse_revision_spec with a target branch posted off a remote branch that is aligned to the same commit as another remote branch.
8) Testing GitClient.parse_revision_spec with a target branch not up-to-date with a remote branch.
9) Testing GitClient.parse_revision_spec with a target branch that has branches from different remotes in its path.Note - The above test descriptions make more sense when we actually view the commit graphs representing each of these scenarios. These commit graphs can be found as part of the documentation of my tests.
Note 2 - The test functions are named in a serial manner (i.e. with suffixes case_one, case_two ... case_nine etc.). This was done because it was not always easy to capture what the test was trying to test in a few words. Hence giving uniform but serial names helped because essentially, all of these tests test the same thing - i.e. whether GitClient.parse_revision_spec can find a good parent, parent_base in each of these scenarios.
I also had to alter one of the existing tests in order to align it with the new algorithm.
| Description | From | Last Updated |
|---|---|---|
|
Col: 55 W291 trailing whitespace |
 reviewbot
|
|
|
Col: 80 E501 line too long (81 > 79 characters) |
reviewbot
|
|
|
Col: 54 E262 inline comment should start with '# ' |
reviewbot
|
|
|
Col: 53 E261 at least two spaces before inline comment |
reviewbot
|
|
|
Col: 55 W291 trailing whitespace |
reviewbot
|
|
|
Col: 80 E501 line too long (86 > 79 characters) |
reviewbot
|
|
|
Col: 55 W291 trailing whitespace |
reviewbot
|
|
|
Col: 55 W291 trailing whitespace |
reviewbot
|
|
|
Col: 80 E501 line too long (86 > 79 characters) |
reviewbot
|
|
|
Col: 61 W291 trailing whitespace |
reviewbot
|
|
|
Col: 80 E501 line too long (86 > 79 characters) |
reviewbot
|
|
|
Col: 55 W291 trailing whitespace |
reviewbot
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
Since youre modifying the docstring for this function, could you reflow it into our document style. e.g. ``` Single line … |
 brennie
|
|
|
Needs a docstirng. |
brennie
|
|
|
Prefer '--remotes=%s' % remote. String interpolation is faster than concatenation in general. |
brennie
|
|
|
Missing a period. "at a commit" ? |
brennie
|
|
|
Again prefer interpolation here. |
brennie
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
Col: 11 W291 trailing whitespace |
reviewbot
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (113 > 79 characters) |
reviewbot
|
|
|
Col: 11 W291 trailing whitespace |
reviewbot
|
|
|
Col: 11 W291 trailing whitespace |
reviewbot
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
Col: 11 W291 trailing whitespace |
reviewbot
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
Col: 28 W291 trailing whitespace |
reviewbot
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
Col: 11 W291 trailing whitespace |
reviewbot
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (113 > 79 characters) |
reviewbot
|
|
|
Col: 11 W291 trailing whitespace |
reviewbot
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
"Parse" |
brennie
|
|
|
Col: 28 W291 trailing whitespace |
reviewbot
|
|
|
"A dictionary with the following keys: |
brennie
|
|
|
Can you reformat this to be a reStructuredText definition list, e.g. ```python """ A dictionary with the following keys: ``'base'`` … |
brennie
|
|
|
Docstrings should be in the following format: """Single line summary Multi-line description """ We also want to use the imperitive … |
brennie
|
|
|
Single quotes for strings. |
brennie
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
Col: 11 W291 trailing whitespace |
reviewbot
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (113 > 79 characters) |
reviewbot
|
|
|
Col: 11 W291 trailing whitespace |
reviewbot
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (82 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
Col: 11 W291 trailing whitespace |
reviewbot
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
Col: 11 W291 trailing whitespace |
reviewbot
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
Col: 11 W291 trailing whitespace |
reviewbot
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
- Testing Done:
-
The aim is to test the algorithm extensively. More specifically, I'm trying to test the algorithm on the 9 conditions outlined in the following comment (https://reviews.reviewboard.org/r/6797/#comment20865). The current tests involve one generic test, and then two tests specifically aimed at testing cases 1 and 2 (all test cases are in reference to the comment). Before I write more tests, I would like feedback on the first two tests and whether they accurately create the conditions they're trying to test.
+ + I also had to alter one of the existing tests in order to align it with the new algorithm.
-
Tool: Pyflakes Processed Files: rbtools/clients/tests.py rbtools/clients/git.py Tool: PEP8 Style Checker Processed Files: rbtools/clients/tests.py rbtools/clients/git.py -
-
-
-
-
-
-
- Change Summary:
-
Updated test desciption and added unit tests
- Testing Done:
-
The aim is to test the algorithm extensively. More specifically, I'm trying to test the algorithm on the 9 conditions outlined in the following comment (https://reviews.reviewboard.org/r/6797/#comment20865). The current tests involve one generic test, and then two tests specifically aimed at testing cases 1 and 2 (all test cases are in reference to the comment). Before I write more tests, I would like feedback on the first two tests and whether they accurately create the conditions they're trying to test.
+ Update - I have now written tests for conditions 3 through 7 also. Still waiting for feedback on my tests. As this stage, feedback from the mentors is crucial for me to make progress.
+ + Note - As of now, the test functions are poorly named (i.e. case one, case two etc.). This is just to signal to the mentors which conditions they're trying to test. I will improve the names once the content of the test cases is apporved by the mentors.
+ I also had to alter one of the existing tests in order to align it with the new algorithm.
- Testing Done:
-
The aim is to test the algorithm extensively. More specifically, I'm trying to test the algorithm on the 9 conditions outlined in the following comment (https://reviews.reviewboard.org/r/6797/#comment20865). The current tests involve one generic test, and then two tests specifically aimed at testing cases 1 and 2 (all test cases are in reference to the comment). Before I write more tests, I would like feedback on the first two tests and whether they accurately create the conditions they're trying to test.
~ Update - I have now written tests for conditions 3 through 7 also. Still waiting for feedback on my tests. As this stage, feedback from the mentors is crucial for me to make progress.
~ Update - I have now written tests for conditions 3 through 6 also. Still waiting for feedback on my tests. As this stage, feedback from the mentors is crucial for me to make progress.
Note - As of now, the test functions are poorly named (i.e. case one, case two etc.). This is just to signal to the mentors which conditions they're trying to test. I will improve the names once the content of the test cases is apporved by the mentors.
I also had to alter one of the existing tests in order to align it with the new algorithm.
-
Hey,
Thanks for putting these tests together. I know you wanted another pair of eyes on these, but given the complexity, I think I want more information shown in the tests in order to help with that. Basically, what a review will need for these tests is a visualization, and documentation.
I see that you have print statements for showing a graph of the resulting branches. We're not going to want those print statements in there for the final review, but I would like to see the output you get for those statements as comments in each test, showing what the tree actually looks like. Sort of in the form of what's in my review on /r/6797/.
That doesn't mean copy/paste from my review. Rather, I want to see the output you get, but formatted in a way that focuses on the important bits, and shown in a comment. It must be kept updated with any changes you put in.
Along with that, please have a description at the top of each test about what you're doing and expecting (similar to my notes in my review), so that we (and future developers) will know for sure what's happening in each test.
The docstrings themselves need to be shorter. The "case 1," "case 2," etc. won't make any sense without knowing about my review, and nobody should care down the road. So, the docstrings should be short, but self-describing, and should be about the function ultimately being tested. So, things like:
Testing GitClient.parse_revision_spec with target branch off a tracking branch aligned with the remote Testing GitClient.parse_revision_spec with target branch off a tracking branch with changes since the remote Testing GitClient.parse_revision_spec with target branch off a branch not properly tracking the remoteetc. So, similar to what I already have as the sections in my review, but written to make it clear that we're dealing with
GitClient.parse_revision_spec.Once that's all done, I'll have a better starting point for verifying that these are doing the right thing.
Thanks!
-
Tool: Pyflakes Processed Files: rbtools/clients/tests.py rbtools/clients/git.py Tool: PEP8 Style Checker Processed Files: rbtools/clients/tests.py rbtools/clients/git.py -
-
-
-
-
-
-
-
-
-
-
-
-
- Testing Done:
-
The aim is to test the algorithm extensively. More specifically, I'm trying to test the algorithm on the 9 conditions outlined in the following comment (https://reviews.reviewboard.org/r/6797/#comment20865). The current tests involve one generic test, and then two tests specifically aimed at testing cases 1 and 2 (all test cases are in reference to the comment). Before I write more tests, I would like feedback on the first two tests and whether they accurately create the conditions they're trying to test.
Update - I have now written tests for conditions 3 through 6 also. Still waiting for feedback on my tests. As this stage, feedback from the mentors is crucial for me to make progress.
+ Update 2 - Documentation and visualization have been added to the tests to aid understanding. I hope this will help the mentors give me feedback.
+ Note - As of now, the test functions are poorly named (i.e. case one, case two etc.). This is just to signal to the mentors which conditions they're trying to test. I will improve the names once the content of the test cases is apporved by the mentors.
I also had to alter one of the existing tests in order to align it with the new algorithm.
- Change Summary:
-
Fixed Issues Opened by Garret (i.e. docstrings and interpolation)
- Diff:
-
Revision 4 (+367 -30)
-
Tool: Pyflakes Processed Files: rbtools/clients/tests.py rbtools/clients/git.py Tool: PEP8 Style Checker Processed Files: rbtools/clients/tests.py rbtools/clients/git.py -
-
-
-
-
-
-
-
-
-
-
- Change Summary:
-
Added the last three unit tests along with documentation/visualization. The unit tests are now complete and waiting on review!
- Diff:
-
Revision 5 (+555 -30)
-
Tool: Pyflakes Processed Files: rbtools/clients/tests.py rbtools/clients/git.py Tool: PEP8 Style Checker Processed Files: rbtools/clients/tests.py rbtools/clients/git.py -
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
Can you reformat this to be a reStructuredText definition list, e.g.
```python
"""A dictionary with the following keys:
``'base'`` (unicode): Description of 'base ``'tip'`` (unicode): Description of tip ``'commit_id'`` (unicode): Description of commit_id"""
-
Docstrings should be in the following format:
"""Single line summary Multi-line description """We also want to use the imperitive mood ("Return") here.
This docstring will also need
ArgsandReturnssections. -
- Change Summary:
-
Fixed stylistic and documentation issues brought up by Barret, ReviewBot
- Summary:
-
[WIP] Adding in a smarter parent-base finding algorithmAdding in a smarter parent-base finding algorithm
- Diff:
-
Revision 6 (+583 -32)
-
Tool: Pyflakes Processed Files: rbtools/clients/tests.py rbtools/clients/git.py Tool: PEP8 Style Checker Processed Files: rbtools/clients/tests.py rbtools/clients/git.py -
-
-
-
-
-
-
-
-
-
-
- Change Summary:
-
Fixed stylistic (line length) issues brought up by ReviewBot
- Diff:
-
Revision 7 (+593 -32)
-
Tool: Pyflakes Processed Files: rbtools/clients/tests.py rbtools/clients/git.py Tool: PEP8 Style Checker Processed Files: rbtools/clients/tests.py rbtools/clients/git.py
- Change Summary:
-
Updated "Testing Done" description
- Testing Done:
-
~ The aim is to test the algorithm extensively. More specifically, I'm trying to test the algorithm on the 9 conditions outlined in the following comment (https://reviews.reviewboard.org/r/6797/#comment20865). The current tests involve one generic test, and then two tests specifically aimed at testing cases 1 and 2 (all test cases are in reference to the comment). Before I write more tests, I would like feedback on the first two tests and whether they accurately create the conditions they're trying to test.
~ The algorithm has been tested extensively. More specifically, GitClient.parse_revision_spec has been tested on the conditions outlined in the following comment (https://reviews.reviewboard.org/r/6797/#comment20865). Detailed documentation and visualization have been added to the tests to aid understanding. I hope this will help a reader understand and review the tests better.
~ Update - I have now written tests for conditions 3 through 6 also. Still waiting for feedback on my tests. As this stage, feedback from the mentors is crucial for me to make progress.
~ Specifically, the conditions being tested are -
~ Update 2 - Documentation and visualization have been added to the tests to aid understanding. I hope this will help the mentors give me feedback.
~ 0) Testing GitClient.parse_revision_spec with target branch off a tracking branch not aligned with the remote.
+ 1) Testing GitClient.parse_revision_spec with target branch off a tracking branch aligned with the remote. + 2) Testing GitClient.parse_revision_spec with target branch off a tracking branch with changes since the remote. + 3) Testing GitClient.parse_revision_spec with target branch off a branch not properly tracking the remote. + 4) Testing GitClient.parse_revision_spec with a target branch that merged a tracking branch off another tracking branch. + 5) Testing GitClient.parse_revision_spec with a target branch posted off a tracking branch that merged another tracking branch. + 6) Testing GitClient.parse_revision_spec with a target branch posted off a remote branch without any tracking branches. + 7) Testing GitClient.parse_revision_spec with a target branch posted off a remote branch that is aligned to the same commit as another remote branch. + 8) Testing GitClient.parse_revision_spec with a target branch not up-to-date with a remote branch. + 9) Testing GitClient.parse_revision_spec with a target branch that has branches from different remotes in its path. ~ Note - As of now, the test functions are poorly named (i.e. case one, case two etc.). This is just to signal to the mentors which conditions they're trying to test. I will improve the names once the content of the test cases is apporved by the mentors.
~ Note - The above test descriptions make more sense when we actually view the commit graphs representing each of these scenarios. These commit graphs can be found as part of the documentation of my tests.
+ + Note 2 - The test functions are named in a serial manner (i.e. with suffixes case_one, case_two ... case_nine etc.). This was done because it was not always easy to capture what the test was trying to test in a few words. Hence giving uniform but serial names helped because essentially, all of these tests test the same thing - i.e. whether GitClient.parse_revision_spec can find a good parent, parent_base in each of these scenarios.
I also had to alter one of the existing tests in order to align it with the new algorithm.
Tool: Pyflakes Processed Files: rbtools/clients/tests.py rbtools/clients/git.py Tool: PEP8 Style Checker Processed Files: rbtools/clients/tests.py rbtools/clients/git.py