Introduce tests for CTree #167

jbduncan · 2017-12-15T00:47:52Z

This is a work-in-progress PR to introduce tests for CTree et al. It partially resolves #83.

The tests are heavily inspired by Guava's own com.google.common.graph tests and borrows their general structure and many other ideas. At time of writing, no effort has been made yet to make the tests go green.

Constructive feedback is welcome!

jbduncan · 2018-01-05T20:57:46Z

Hey @jrtom, my PR here is not done yet, as I still have to write tests for CTreeNetwork (CTree should be tested extensively now), but I would greatly appreciate any feedback you may have at this point!

jbduncan · 2018-01-05T21:03:18Z

(Oh oops, seems that CTree#{root,depth,height] haven't been tested yet. But I'll tackle those during or after giving CTreeNetwork a go.)

jbduncan · 2018-01-05T22:09:12Z

(Actually, I've realised that I do not have the time and energy to tackle testing CTreeNetwork, so I'll leave that task to one side.)

jbduncan · 2018-01-05T22:13:46Z

Because of a number of uber commits which were merged into jung/master recently, it's proving to be too difficult for me to resolve conflicts, even with the built-in GitHub "Resolve conflicts" button, so I'm closing this PR with the intent to re-open it afresh atop a recent version of jung/master.

jrtom · 2018-01-05T22:57:30Z

Can you tell me what conflicts you were encountering? Your latest commit passed the tests, so while I haven't looked in detail at all the changes in this PR, we might be able to go ahead and merge what you have if everything looks OK.

(And then you can take on CTreeNetwork at another time if that works for you.)

jbduncan · 2018-01-05T23:28:19Z

@jrtom It's strange - I was having conflicts with ObservableNetwork and some other file (I cannot remember which now). Both files had a lot of changes to do with the recent "vertex" to "node" normalisation. And no matter whether I used git rebase or the GitHub "Resolve conflicts" button, I ended up either with git saying that I had ~87 commits to push instead of ~8 (from rebasing), or the conflicts apparently weren't resolved for some bizarre reason (from using the "Resolve conflicts" button).

It's funny that the tests only pass only now. I could've sworn there was still a merge conflict after I resolved things using "Resolve conflicts". I'll re-open this PR then, as things seem to have resolved (haha) themselves. :)

jbduncan · 2018-01-05T23:29:18Z

Oh no, wait, the merge conflict seems to be back?

This is doing my head in. :/

jbduncan · 2018-01-05T23:33:04Z

@jrtom If it's alright with you, I think I'll close this PR again. I went and opened #172 before I saw your earlier message, and #172 doesn't seem to suffer from this merge conflict problem, so I think it should be easier to review and merge and this PR.

jrtom · 2018-01-05T23:35:07Z

Probably the easiest way to resolve this would be to revert the changes to those two files, which are unrelated textual cleanup changes anyway.

(Side point: I'm not sure it's helpful for ObservableNetwork to be doing all those null checks, unless we're presuming that the delegate won't be doing them...and in the case of the common.graph implementations, it is.)

jbduncan · 2018-01-05T23:56:09Z

Probably the easiest way to resolve this would be to revert the changes to those two files, which are unrelated textual cleanup changes anyway.

Okay, that sounds like a way forward to me. Were you thinking of reverting the changes to those files youself? If not, I could open a PR myself, but it would have to wait until tomorrow earliest, as I need to get to bed soon. :)

(Side point: I'm not sure it's helpful for ObservableNetwork to be doing all those null checks, unless we're presuming that the delegate won't be doing them...and in the case of the common.graph implementations, it is.)

I personally think that the null checks are justified for the ObservableNetwork constructor and *GraphEvent* methods, as AFAICT those methods would otherwise allow nulls to silently pass-through. But for all the other methods, I think you make a good point. So if you have no objections, I will remove the null checks for all the methods except the constructor and *GraphEvent* methods.

jbduncan · 2018-01-07T18:54:52Z

@jrtom I've managed to merge jung/master into this PR after a couple of tries, so there's no need to temporarily revert changes to ObservableNetwork and ParallelEdgeIndexFunction now. :)

jbduncan · 2018-01-07T19:40:40Z

@jrtom I believe that I've fixed all test errors reported by Travis, so I'm ready for another review whenever you are. :)

FYI, before merging, I'd quite like to squash all my commits, so unless you can squash them on your end, I'd like to be given time after the next review to squash them myself.

tomnelson · 2018-01-08T12:07:53Z

jung-api/src/test/java/edu/uci/ics/jung/graph/CTreeEquivalenceTest.java

+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */


Is that the correct license and copyright if it's going into JUNG?

Indeed it isn't, good catch! I'll address this later today.

jrtom · 2018-01-09T05:29:28Z

I'll take a look at this in the next day or so.

Regarding commit-squashing, that's easy to do as part of the merge, so just remind me before I do the merge (i.e., when it looks like we're done addressing comments).

jbduncan · 2018-01-14T18:50:52Z

Hi @jrtom, how are you doing with reviewing this PR? Do you need anything from my end, or do you just need more time to look through things? :)

jrtom · 2018-01-16T00:59:37Z

Taking a look now.

By the way, thanks for taking a look at @tomnelson's layout events change CL. :)

jrtom

Basically looks good. Thanks for taking this on. :)

I made one minor suggestion but we can do that in a separate pass.

jrtom · 2018-01-16T01:08:02Z

jung-api/src/main/java/edu/uci/ics/jung/graph/DelegateCTree.java

@@ -24,18 +28,16 @@
 import java.util.Optional;
 import java.util.Set;

-class DelegateCTree<N> implements MutableCTree<N> {
+class DelegateCTree<N> extends AbstractGraph<N> implements MutableCTree<N> {


good catch :)

jrtom · 2018-01-16T01:14:28Z

jung-api/src/main/java/edu/uci/ics/jung/graph/DelegateCTree.java

        : Optional.of(Iterables.getOnlyElement(predecessors));
  }

  @Override
  public int depth(N node) {
-    Preconditions.checkArgument(delegate.nodes().contains(node));
+    checkNotNull(node, "node");


You've got a bunch of identical checks in the accessors. Consider creating a private method to sanity check the input:

private void sanityCheckNode(N node) { checkNotNull(node, "node"); checkArgument(delegate.nodes().contains(node), NODE_NOT_IN_TREE, node); }

That both reduces the amount of code and makes it easier for us to uniformly change our error handling if desired.

Good catch! I'll try to remember to address this if I find the time to improve the tests. :)

jrtom · 2018-01-16T01:21:05Z

jung-api/src/test/java/edu/uci/ics/jung/graph/AbstractCTreeTest.java

+  }
+
+  static <N> void validateTree(CTree<N> tree) {
+    // TODO: Replace Graphs#copyOf with a tree-specific copyOf method if it ever turns up


I'm not sure what might cause us to write a tree-specific version of this method. Did you have anything specific in mind, i.e., how might that be useful?

Oops, I've just realised that I never responded to this comment. Sorry @jrtom!

I think I was concerned that using Graphs#copyOf forced one of the parameters of assertStronglyEquivalent to be of type Graph instead of CTree. If I remember correctly, the way I saw things then, this prevented the test from checking that if a CTree was copied then it's root() and height() remained the same and that the depth()s and predecessor()s of each of its nodes remained unchanged.

But having said this, TreeUtils doesn't have a copyOf method AFAICT, and it's unclear to me if there ever will be such a method.

Should I expand this TODO comment?
Or do you think it's not worth the effort and that I should just remove it (and in the process remove this TODO as well)?

jbduncan · 2018-04-19T18:05:05Z

@jrtom, we discussed privately that my PR here caused a breakage somewhere in JUNG (I think with one of the demos managed by @tomnelson specifically), and that I'd raise a new PR to fix it, but I can't seem to find that breakage any longer.

@tomnelson @jrtom Would you kindly point me in the direction of the message that mentioned the breakage, or would you kindly remind me what the breakage was?

jbduncan · 2018-04-19T18:22:47Z

Oh, I found the messages regarding the breakages in my emails:

@jbduncan the change to DelegateCTreeNetwork::setDepth broke the TreeCollapseDemo. You might want to have a look at it to make sure it does what you want.

and:

Also, the new Preconditions checks in TreeUtils make it impossible to re-expand the root node once you collapse there. I'll look into that more tomorrow.
Just run TreeCollapseDemo and select various nodes to collapse then expand. You'll see where the problems are.

I can't seem to reproduce message 1 regarding DelegateCTreeNetwork::setDepth. @tomnelson Could you clarify for me what I need to do to see the respective breakage in TreeCollapseDemo?

I have been able to reproduce message 2 regarding TreeUtils; it looks like TreeUtils::addSubTree throws a NullPointerException because it doesn't expect parameter subTreeParent to be null. But I'm struggling to understand why null is being passed to it, and why null should be a valid value. @tomnelson Could you clarify things for me?

jbduncan · 2018-04-27T20:58:20Z

Message 1 has been fixed in #202. Message 2 is being addressed in #204.

jbduncan added 6 commits December 15, 2017 00:41

WIP

420b701

WIP - start making tests pass

4ab487e

WIP - test towards jrtom#169

b1c0b4f

WIP - add/amend more tests; make most pass (at least locally)

b40bbb8

WIP - Add TODOs

2209ed2

WIP

08a500c

jbduncan added 2 commits January 5, 2018 21:16

WIP

ebbf561

WIP - remove no-longer-applicable test

d3bedbb

jbduncan force-pushed the tree-tests branch from 9229085 to d3bedbb Compare January 5, 2018 22:08

jbduncan closed this Jan 5, 2018

jbduncan mentioned this pull request Jan 5, 2018

Introduce tests for CTree #172

Closed

jbduncan reopened this Jan 5, 2018

jbduncan closed this Jan 5, 2018

jbduncan reopened this Jan 5, 2018

jbduncan added 5 commits January 5, 2018 23:59

WIP - Remove unnecessary null checks

65fbfd1

WIP - remove commented-out code block

4a06fd6

WIP - add more tests

82b148c

Merge branch 'master' into tree-tests

9ea1e6d

Merge branch 'master' into tree-tests

294e955

WIP - fix tests

eb4287b

tomnelson reviewed Jan 8, 2018

View reviewed changes

WIP - correct licenses and do minor test cleanup

4eb10c6

jrtom reviewed Jan 16, 2018

View reviewed changes

jrtom merged commit a5de353 into jrtom:master Jan 16, 2018

jbduncan mentioned this pull request Apr 19, 2018

Make CTreeNetwork#setDepth like CTree#setDepth #202

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce tests for CTree #167

Introduce tests for CTree #167

jbduncan commented Dec 15, 2017 •

edited

Loading

jbduncan commented Jan 5, 2018

jbduncan commented Jan 5, 2018

jbduncan commented Jan 5, 2018

jbduncan commented Jan 5, 2018

jrtom commented Jan 5, 2018

jbduncan commented Jan 5, 2018

jbduncan commented Jan 5, 2018

jbduncan commented Jan 5, 2018

jrtom commented Jan 5, 2018

jbduncan commented Jan 5, 2018

jbduncan commented Jan 7, 2018

jbduncan commented Jan 7, 2018

tomnelson Jan 8, 2018

jrtom Jan 9, 2018

jbduncan Jan 9, 2018

jrtom commented Jan 9, 2018

jbduncan commented Jan 14, 2018

jrtom commented Jan 16, 2018

jrtom left a comment

jrtom Jan 16, 2018

jbduncan Feb 3, 2018

jrtom Jan 16, 2018

jbduncan Feb 3, 2018

jrtom Jan 16, 2018

jbduncan Apr 12, 2018

jbduncan commented Apr 19, 2018

jbduncan commented Apr 19, 2018

jbduncan commented Apr 27, 2018

Introduce tests for CTree #167

Introduce tests for CTree #167

Conversation

jbduncan commented Dec 15, 2017 • edited Loading

jbduncan commented Jan 5, 2018

jbduncan commented Jan 5, 2018

jbduncan commented Jan 5, 2018

jbduncan commented Jan 5, 2018

jrtom commented Jan 5, 2018

jbduncan commented Jan 5, 2018

jbduncan commented Jan 5, 2018

jbduncan commented Jan 5, 2018

jrtom commented Jan 5, 2018

jbduncan commented Jan 5, 2018

jbduncan commented Jan 7, 2018

jbduncan commented Jan 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrtom commented Jan 9, 2018

jbduncan commented Jan 14, 2018

jrtom commented Jan 16, 2018

jrtom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbduncan commented Apr 19, 2018

jbduncan commented Apr 19, 2018

jbduncan commented Apr 27, 2018

jbduncan commented Dec 15, 2017 •

edited

Loading