-
-
Notifications
You must be signed in to change notification settings - Fork 131
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
Feature - Vitest test suite for utility functions #137
Feature - Vitest test suite for utility functions #137
Conversation
…mand, adding 2 different test commands (one for ci, one for coverage but both will perform the same tests), update various ignore lists so we don't lint or spell check test output files
✅ Deploy Preview for quicksnip ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, but i would prefer for tests to be in the /tests
folder instead of mixed with the source code
Thanks for reviewing. Personally I prefer to keep all files together (this could be the .ts|.tsx, the .test.ts|.tsx, the .css|.scss, the .stories, the .md (if applicable), etc.) within the same directory as (for me) it makes it much easier to find all associated files. Placing all test files within the It is a matter of preference though, so let me know your thoughts. Thanks |
IMO the repo shouldn't grow that much bigger, it's still a small project, appart from the snippet quantity |
@Mathys-Gasnier In that case we'll keep it simple. All test files are now contained within the |
Btw could you add more tests to your PR, so we don't merge just to merge one test and "forget" about it ? |
I'm not sure what you mean - As for forgetting about tests - I think most contributors would also want to test a util function when added or modified so I do not think this will be a problem in the long run. Even if this occurs when drafting a PR the reviewers would catch it. The first line of the checklist also requests that contributors test their code; perhaps the language here could be extended to encompass the addition of unit tests where applicable? |
There is the utils root folder, with the scripts to check/consolidate snippets, but one of the file |
Let me see what I can do. |
…ding additional unit tests
@Mathys-Gasnier I've made a lot of changes to try and expand the test suite. Firstly, I've added the tsx package and changed the scripts within the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Impressive work, didn't knew tsx
was a thing, I always use ts-node
, some little changes, but overall really good work
Oh and forgot, why did changing to TS change the consolidated snippets ? it's just a change of order of the fields, but i'm not a big fan that it changes that |
I reverted those as I did not mean for that to happen. |
Thanks, I think the |
Once you are done, could you ping me on discord ? So i can clone your branch and test the scripts are working correctly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really good work, let's wait on another review and I think we are good to merge
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. 👍
Updated and ready for merge. I have tested the scripts locally but it might be worth checking these again as the scripts have changed since they were modified. |
Description
vitest
package.utils/slugify.ts
utility function.slugify
function so that unnecessary escape chars are removed (function tested before and after the changes using the tests).pre-commit
scripts (including the GitHub workflow script) to include this new test command.Type of Change
Checklist
Related Issues
Closes #135
Additional Context
Screenshots (Optional)
Click to view screenshots