Add tests for tree reset propagation #60

thepaperpilot · 2024-02-13T14:26:11Z

thepaperpilot commented

2024-02-13 14:26:11 +00:00

Tests will fail until #57 is merged in.

thepaperpilot force-pushed feat/reset-tests from 7a7f95bc7b to 1f60960e5b

2024-02-13 14:42:24 +00:00

Compare

thepaperpilot added 1 commit 2024-02-14 18:06:59 +00:00

Merge branch 'main' into feat/reset-tests

Run Tests / test (pull_request) Failing after 2m7s

Details

08b1cf6368

escapee reviewed 2024-02-21 02:05:22 +00:00

tests/features/tree.test.ts

					
				@ -0,0 +54,4 @@

				        }));

				        tree.reset(b);

				        expect(shouldNotReset.value).toBe(false);

				    });

escapee commented

2024-02-21 02:01:33 +00:00

This one isn't feeling right - isn't tree.reset(node) supposed to trigger that node's reset too, or is tree.reset(node) supposed to just start a reset that propagates from that node, but doesn't include it?

This one isn't feeling right - isn't `tree.reset(node)` supposed to trigger that node's `reset` too, or is `tree.reset(node)` supposed to just start a reset that propagates from that node, but doesn't include it?

thepaperpilot commented

2024-02-21 04:08:18 +00:00

I based this off the behavior based on the createResetButton util which calls reset on the node itself and then propagates the rest. The idea being that you're resetting for currency on that layer, not resetting that layer. This has been a point of contention previously though so I'm open to changing it in order to be more clear and intuitive, but that'll be a breaking change that's out of scope for this PR.

I based this off the behavior based on the `createResetButton` util which calls reset on the node itself and then propagates the rest. The idea being that you're resetting for currency on that layer, not resetting that layer. This has been a point of contention previously though so I'm open to changing it in order to be more clear and intuitive, but that'll be a breaking change that's out of scope for this PR.

escapee commented

2024-02-21 04:11:06 +00:00

Fair enough - should be fine to just talk about that in the features' docs, nothing that needs to change here then.

escapee marked this conversation as resolved

thepaperpilot added 1 commit 2024-02-21 04:08:27 +00:00

Merge branch 'main' into feat/reset-tests

Run Tests / test (pull_request) Successful in 2m2s

Details

ccdf6f0c93

escapee approved these changes 2024-02-21 04:11:21 +00:00

thepaperpilot added 1 commit 2024-02-21 04:11:42 +00:00

Merge branch 'main' into feat/reset-tests