Forest unit tests by mcrvaz · Pull Request #193 · DioxusLabs/taffy

mcrvaz · 2022-06-26T04:21:58Z

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_capacity tests, since it sometimes ignores the argument and simply initializes the Forest with a fixed 256 nodes capacity. (see sys.rs)

I've removed compute_layout from the implementation in forest.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.

Weibye

Great work so far!

Since no layout is being calculated in any of these tests, I think we can just remove any custom defined layout like flex_grow and just keep everything to Default

Weibye · 2022-06-26T09:01:41Z

src/forest.rs

+    }
+
+    #[test]
+    fn mark_dirty() {


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

I've named it mark_dirty_propagates_to_parents, should be clearer now.

Weibye · 2022-06-26T09:10:24Z

src/forest.rs

+    fn clear() {
+        let mut forest = Forest::with_capacity(1);
+        add_default_leaf(&mut forest);
+        forest.clear();
+        assert_forest_size(&forest, 0);
+    }


Does adding an assert after the setup and before calling the methods to test help convey more precisely what is being tested?

Suggested change

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

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_leaf is already tested and will fail if it doesn't do what it's supposed to do.

If this test fails because new_leaf behaved 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 this clear one.

But anyway, if you think this would make our test suite more standardized, let me know and I'll change it :)

I agree with your stance here @mcrvaz. Being able to isolate exactly what failed from unit tests is incredibly valuable.

Weibye · 2022-06-26T09:11:17Z

src/forest.rs

+        let s1 = FlexboxLayout { flex_grow: 1.0, ..Default::default() };
+        let s2 = FlexboxLayout { flex_grow: 2.0, ..Default::default() };


Is the grow property relevant for this test?

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?

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.

Weibye · 2022-06-26T09:19:22Z

src/forest.rs

+    use crate::style::FlexboxLayout;
+    use crate::sys::new_vec_with_capacity;
+
+    fn assert_forest_size(forest: &Forest, size: usize) {


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?

So, this is one of the quirks of this data structure, actually the vectors must always have the same length.

nodes with 3 elements containing their NodeData indexed by NodeId

parents contain one entry for each node, also indexed by NodeId, 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.

children works the same as the parent, 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?

I would like that method :) Can you also leave a comment documenting this weird behavior?

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.

Excellent, I like those changes.

src/forest.rs

alice-i-cecile

I'm happy to merge this once the warning is fixed. Nice work :)

…nto forest-unit-tests

* 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 <[email protected]>

mcrvaz added 2 commits June 26, 2022 00:34

Forest unit tests

f114c81

Removing old Forest compute_layout uses

f9f7208

Weibye reviewed Jun 26, 2022

View reviewed changes

alice-i-cecile added the code quality Make the code cleaner or prettier. label Jun 26, 2022

mcrvaz added 2 commits June 28, 2022 00:31

Forest unit tests additions and refactoring

3199045

Code style adjustments

ede2f47

mcrvaz commented Jun 28, 2022

View reviewed changes

src/forest.rs Show resolved Hide resolved

mcrvaz marked this pull request as ready for review June 28, 2022 03:52

Merge branch 'main' into forest-unit-tests

e49f635

alice-i-cecile reviewed Jun 28, 2022

View reviewed changes

mcrvaz added 2 commits June 28, 2022 22:42

Removing Forest node_measure_eq warning

7d760bd

Merge branch 'forest-unit-tests' of https://github.com/mcrvaz/taffy i…

6b276e6

…nto forest-unit-tests

alice-i-cecile merged commit 560e849 into DioxusLabs:main Jun 29, 2022

		let s1 = FlexboxLayout { flex_grow: 1.0, ..Default::default() };
		let s2 = FlexboxLayout { flex_grow: 2.0, ..Default::default() };

Conversation

mcrvaz commented Jun 26, 2022

Objective

Context

Feedback wanted

Uh oh!

Weibye left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcrvaz Jun 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcrvaz Jun 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alice-i-cecile left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mcrvaz Jun 28, 2022 •

edited

Loading

mcrvaz Jun 27, 2022 •

edited

Loading