Fix removing old nodes from parent

[mc1098]

Fixes #427

The #427 bug was introduced in #416 inside the reconcile_fragments function.

The quick fix was to add the negation that got dropped during that change - this seems
to have caused the bug.

I instead extracted the removal of nodes from the parent node part of the
reconcile_fragments function to a new function and added tests for this new
function to avoid any future regressions.

The tests use a pretty rough/bare bones mock of a GenericNode in order to satisfy the
trait bounds required by the extracted function. I think the tests are worth the added file
noise of the mock but let me know if you'd prefer I remove it (and probably the extracted
function at that point).

[lukechu10]

Wow that was fast. Thanks! will review soon

[codecov]

Codecov Report

Merging #428 (7763073) into master (94eef35) will not change coverage.
The diff coverage is 0.00%.

@@           Coverage Diff           @@
##           master     #428   +/-   ##
=======================================
  Coverage   64.96%   64.96%           
=======================================
  Files          52       52           
  Lines        8278     8278           
=======================================
  Hits         5378     5378           
  Misses       2900     2900           

Impacted Files	Coverage Δ
packages/sycamore-core/src/render.rs	58.95% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 94eef35...7763073. Read the comment docs.

[mc1098]

Oh the empty mock functions are causing the decrease in coverage 😞

[lukechu10]

I think instead of creating a new MockNode, it would be cleaner to just re-use SsrNode which is what most other tests do. Since sycamore depends on sycamore-core, we can't add sycamore as a dependency but we can add it as a dev-dependency. Unfortunately, rust-analyzer doesn't seem to recognize that but at least it compiles fine.

[mc1098]

I think instead of creating a new MockNode, it would be cleaner to just re-use SsrNode which is what most other tests do. Since sycamore depends on sycamore-core, we can't add sycamore as a dependency but we can add it as a dev-dependency. Unfortunately, rust-analyzer doesn't seem to recognize that but at least it compiles fine.

Cleaner yes, but I worry about adding point of failures to a unit test that have nothing to do with the function under test - this test may fail because of changes made in the sycamore-web which seems wrong imo.

Also I seem to have trouble with compiling the test as the GenericNode bound in the new function is not matching the generic_node::GenericNode used with the SsrNode - I have tried adding the ssr feature to the sycamore crate dev-dependency which already exists and adding the sycamore-web crate (with feature) directly but no dice (this is rustc rejecting it and not just rust-analyzer for me).

The other points I've done locally so no issue there :)

[lukechu10]

Cleaner yes, but I worry about adding point of failures to a unit test that have nothing to do with the function under test - this test may fail because of changes made in the sycamore-web which seems wrong imo.

Yeah that's a valid concern but since none of the methods for GenericNode are actually called (since all the methods in MockNode are basically stubs), changes to sycamore-web shouldn't affect this. As for the error, that's a bit weird. I'll try to take a look soon.

[mc1098]

If you prefer, I can change this PR to do the simple bug fix (add the !) and maybe just add tests on the extracted if condition, if you want the regression check retained.

The motivation for the mocking was as a future base for further testing the reconcile_fragments function, and possibly others, but this can be moved into another PR that may tackle the refactoring of that function entirely instead.

[lukechu10]

If you prefer, I can change this PR to do the simple bug fix (add the !) and maybe just add tests on the extracted if condition, if you want the regression check retained.

The motivation for the mocking was as a future base for further testing the reconcile_fragments function, and possibly others, but this can be moved into another PR that may tackle the refactoring of that function entirely instead.

Alright that sounds like a good idea. Also there are some tests for reconcile_fragments already in the ssr integration tests (packages/sycamore/tests/ssr/reconcile.rs) if you want to continue working on this.