Forest unit tests #193
Forest unit tests #193
Conversation
src/forest.rs
Outdated
| } | ||
|
|
||
| #[test] | ||
| fn mark_dirty() { |
There was a problem hiding this comment.
This test verifies that dirty-propagation works as expected. (Marking parent as dirty, should mark all children as dirty). I'm not sure we need another test just for "setting node to dirty, should have the node report as dirty" but I do think we should either rename the test or add a comment conveying that tests dirty-propagation
There was a problem hiding this comment.
I've named it mark_dirty_propagates_to_parents, should be clearer now.
| fn clear() { | ||
| let mut forest = Forest::with_capacity(1); | ||
| add_default_leaf(&mut forest); | ||
| forest.clear(); | ||
| assert_forest_size(&forest, 0); | ||
| } |
There was a problem hiding this comment.
Does adding an assert after the setup and before calling the methods to test help convey more precisely what is being tested?
| fn clear() { | |
| let mut forest = Forest::with_capacity(1); | |
| add_default_leaf(&mut forest); | |
| forest.clear(); | |
| assert_forest_size(&forest, 0); | |
| } | |
| fn clear() { | |
| let mut forest = Forest::with_capacity(1); | |
| add_default_leaf(&mut forest); | |
| assert_forest_size(&forest, 1); // <- Before something changes | |
| forest.clear(); | |
| assert_forest_size(&forest, 0); // <- This should be the result of the change | |
| } |
I personally like doing this and I find it help with conveying intent, but it might work best on very narrow tests like this and be more confusing on tests with many asserts to capture the full picture
There was a problem hiding this comment.
While I do think that this make the intent clearer in this case, I'm a bit against it. I see some problems by doing this:
new_leafis already tested and will fail if it doesn't do what it's supposed to do.- If this test fails because
new_leafbehaved badly we'll have two broken tests, instead of just one which would be telling us exactly what went wrong. - And personally it bothers me a little that it breaks the Arrange, Act, Assert pattern.
All of these points also apply to other test cases, not only thisclearone.
But anyway, if you think this would make our test suite more standardized, let me know and I'll change it :)
There was a problem hiding this comment.
I agree with your stance here @mcrvaz. Being able to isolate exactly what failed from unit tests is incredibly valuable.
src/forest.rs
Outdated
| let s1 = FlexboxLayout { flex_grow: 1.0, ..Default::default() }; | ||
| let s2 = FlexboxLayout { flex_grow: 2.0, ..Default::default() }; |
There was a problem hiding this comment.
Is the grow property relevant for this test?
There was a problem hiding this comment.
This was just supposed to be some non-default value for FlexboxLayout, so it can be compared later. Any suggestions on making the intent more clear here?
There was a problem hiding this comment.
I've created an utils function for generating a non-default FlexboxLayout with a comment indicating that it's used for comparing NodeData. This wouldn't be necessary if NodeData implemented some kind of equality, but I'd rather do this in another issue.
| use crate::style::FlexboxLayout; | ||
| use crate::sys::new_vec_with_capacity; | ||
|
|
||
| fn assert_forest_size(forest: &Forest, size: usize) { |
There was a problem hiding this comment.
If you have 1 parent with 2 children in the tree would the length of the vectors return:
Nodes: 3
Children: 2
Parents: 1
Or do they report their capacity?
There was a problem hiding this comment.
So, this is one of the quirks of this data structure, actually the vectors must always have the same length.
nodeswith 3 elements containing theirNodeDataindexed byNodeIdparentscontain one entry for each node, also indexed byNodeId, and inside each entry there's another vector storing all of the elements' parents. That means that every children would have an entry here ponting to their single parent.childrenworks the same as theparent, just flipped accordingly.- Those inner vectors are not indexed by
NodeId, just stored as they come.
In the case you've presented:
children[parent_id] would have len == 2
children[first_child_id] would be empty
children[second_child_id] would be empty
parents[parent_id] would be empty
parents[first_child_id] would have len == 1
parents[second_child_id] would have len == 1
Maybe it would make sense to just create a new function in Forest that reports the node count?
There was a problem hiding this comment.
I would like that method :) Can you also leave a comment documenting this weird behavior?
There was a problem hiding this comment.
I've create a len function and placed some comments indicating that all of them must always have the same length.
For the tests I've kept this assert_forest_size which I think is a more robust way of asserting the forest consistency.
There was a problem hiding this comment.
Excellent, I like those changes.
* Forest unit tests * Removing old Forest compute_layout uses * Forest unit tests additions and refactoring * Code style adjustments * Removing Forest node_measure_eq warning Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Objective
Fixes #176
Context
As discussed in #176, I've skipped any tests related to multi-parented nodes.
Also related to the allocation limitations in some environments, I've skipped the
create_with_capacitytests, since it sometimes ignores the argument and simply initializes theForestwith a fixed 256 nodes capacity. (see sys.rs)I've removed
compute_layoutfrom the implementation inforest.rs, since it wasn't doing anything and I don't think that this was the correct place for it.I'll later create a new issue with some usability improvements suggestions.
Feedback wanted
I'll open this as a draft because I still wanna do a refactoring pass, but still would love to know what you guys think and what can be improved.