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

test(nodeOps): rename file, fix type errors, add tests #637

Merged
merged 8 commits into from
Apr 26, 2024

Conversation

andretchen0
Copy link
Contributor

@andretchen0 andretchen0 commented Apr 12, 2024

Problem

src/core/nodeOps.ts has a test file, but it's called nodeOpts.test.ts – note the extra 't'.

Solution

Rename to src/core/nodeOps.test.ts.

Problem

The test file had a lot of type errors.

Solution

Fix most type errors.

Problem

The test file was organized in a flat hierarchy, leading to a lot of repetition of names createElement should ....

Solution

Group tests using describe

Problem

There's no way to easily see test coverage.

Solution

Add test coverage plugin to Vitest and add to pnpm run test:ui script.

Here's how to use it in the UI.

Problem

nodeOps test coverage is about 50%, which makes changes riskier than they could be.

Solution

Improve test coverage


closes #631

Copy link

netlify bot commented Apr 12, 2024

Deploy Preview for tresjs-docs ready!

Name Link
🔨 Latest commit 6fd8c08
🔍 Latest deploy log https://app.netlify.com/sites/tresjs-docs/deploys/661a3571d613c40008640ab5
😎 Deploy Preview https://deploy-preview-637--tresjs-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@andretchen0 andretchen0 removed the request for review from alvarosabu April 13, 2024 06:32
@andretchen0 andretchen0 changed the title test(nodeOps): rename file, fix type errors and organize test(nodeOps): rename file, fix type errors, add tests Apr 13, 2024
@andretchen0
Copy link
Contributor Author

Hey @alvarosabu !

I increased test coverage of nodeOps to about 90%.

  • Could you have a look and see if anything stands out: Do the tests reflect the intention of the code?
  • I'm not sure what to do about things like deregisterAtPointerEventHandlerIfRequired. It was said that these were going away, so I didn't write tests. Is that right?
  • There are still gaps, but fewer and smaller. Feel free to suggest more tests.

@alvarosabu
Copy link
Member

Hi @andretchen0 thanks a lot for submitting this PR, I'm so happy to have proper coverage, especially on the nodeOps. This will make our maintenance easier.

I'm not sure what to do about things like deregisterAtPointerEventHandlerIfRequired. It was said that these were going away, so I didn't write tests. Is that right?

Yes, that is going to disappear (check #515)

You would need to change the origin to v4 instead of main since you branch from v4. There are a lot of changes in this PR that don't belong.

@andretchen0 andretchen0 changed the base branch from main to v4 April 14, 2024 17:59
@andretchen0
Copy link
Contributor Author

@alvarosabu

v4

Whoops! Thanks for the heads up! Fixed.

@andretchen0 andretchen0 marked this pull request as ready for review April 15, 2024 11:03
@andretchen0 andretchen0 merged commit 1121eb1 into v4 Apr 26, 2024
3 checks passed
@andretchen0 andretchen0 deleted the test/nodeOps-organize branch April 26, 2024 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Better test coverage for core
2 participants