search

Tresjs / tres

Declarative ThreeJS using Vue Components
https://tresjs.org
MIT License
2.17k stars 104 forks source link

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

Closed andretchen0 closed 5 months ago

andretchen0 commented 5 months ago

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

netlify[bot] commented 5 months ago

Deploy Preview for tresjs-docs ready!

Name Link
Latest commit 6fd8c08b547144734ebf638e44153e4285951c98
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 commented 5 months ago

Hey @alvarosabu !

I increased test coverage of nodeOps to about 90%.

alvarosabu commented 5 months ago

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 commented 5 months ago

@alvarosabu

v4

Whoops! Thanks for the heads up! Fixed.