Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve test coverage #87

Merged
merged 1 commit into from
Jan 10, 2025
Merged

Conversation

MichaelWest22
Copy link
Collaborator

Been going through the built in loc coverage reports to help identify any areas where the existing tests are not covering all code branches. I found a few things we could to improve the coverage and found a few edge cases not tested or handled well in the process.

Findings:

  • isIdSetMatch has Null checks mirroring isSoftMatch but I was unable to trigger these null checks because the two callers of this function already scan for Nulls. This means it is best to remove the null checks and update the jsdocs to enforce only non null Node inputs and this also resolves the code coverage issue here
  • parseContent has null return path if parsed content firstChild could be null but this does not seem possible here. generatedByIdiomorph.add() wants a null check before adding which I can understand even though it should never happen but we can move the return outside of this check and have just one return to return the null if it is ever needed which solves the code coverage issue here
  • findIdSetMatch has a feature to not match content with ID's if matching it would cause more future nodes to discard more ids than in this node. This path was never being tested I found So added a test so this is now covered to avoid future regressions
  • head morphing I found was very broken in the test because idiomorph currently has no support to target replacing just the head and always generates body tags when generating fragments and the old block test was just replacing the whole page wiping everything out. I rewrote two tests to do the script execution testing with included existing body content to allow real page morphing to be completed. We could add a feature in the future maybe if it was every needed to support a head only option like the block option except it does not perform the second main page morph.
  • htmx extension has a isInlineSwap function that was never getting tested. This is because it is only used by oob-swap's so I added two oob swap tests. I found while hx-swap-oob="moprh" works as expected it is currently not possible to perform morph:innerHTML oob swaps as htmx oob-swap feature uses the first colon it finds as a selector delimitator. I've commented out my inner test for possible future use. We could add a "morphInnerHTML" or "morphInner" swap style option to the extension easily which I tested and works really well.

Note that test coverage is not at 100% yet after this change alone because there are a few other paths that are fixed in the singlePass proposed future test changes and when i tested with this branch change i was able to get to 100%!!!!!

@botandrose
Copy link
Collaborator

This gets a big thumbs up from me, and I would merge, except for the htmx integration tests, which I'm not qualified to evaluate. @1cg can you review?

@1cg 1cg merged commit 7dccbce into bigskysoftware:main Jan 10, 2025
4 checks passed
@1cg
Copy link
Contributor

1cg commented Jan 10, 2025

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants