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(__tests__ dir): added vitest and coverage reporting GitHub Actions workflow #8

Merged
merged 8 commits into from
Jul 29, 2024
Merged

test(__tests__ dir): added vitest and coverage reporting GitHub Actions workflow #8

merged 8 commits into from
Jul 29, 2024

Conversation

KemingHe
Copy link
Contributor

@KemingHe KemingHe commented Jul 26, 2024

Deliverables

Mon. 07/29 Update

@ad1992 Git hard reset to only implement testing, refactored naming 3rd time in e3264f8.

Sun. 07/28 Update

@ad1992 , done 10

Added query result caching via private cache: { [query: string]: Result };.
Added and maintained 100% test coverage of cache hit and miss.

For details, see: c476214

Fri. 07/26 Update

@ad1992 , done #3

  • Unittest 100% statement coverage for utils.ts and Fuzzy.ts.
  • Vitest benchmarking utils.ts performance (see package.json and __tests__/benchmark.json)
  • GitHub Actions workflow node-ci.yml to auto yarn install and test per push/PR.

MOST IMPORTANT

  • ZERO functional code changes.

Screenshot of GitHub (auto-trigger) Actions run output:

Screenshot from 2024-07-26 16-01-44

Copy link

vercel bot commented Jul 26, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
fuzzify ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 29, 2024 4:40pm

@KemingHe
Copy link
Contributor Author

@ad1992 Let me know how you like the yarn test:bench:compare feature. I will let you decide how to incorporate it into the contrib workflow (prob an edit to README.md) so that future devs doesn't break performance on top of functionality. ☀️

@ad1992
Copy link
Owner

ad1992 commented Jul 26, 2024

Ohh wow, this PR looks great @KemingHe, very happy to see the tests being added so soon!
I will be reviewing this PR tomorrow

Copy link
Owner

@ad1992 ad1992 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @KemingHe for the PR ✨

I am thinking should we just add coverage instead of benchmarks ? wdyt ?
This one I mean. - https://github.com/davelosert/vitest-coverage-report-action

__tests__/utils.test.ts Outdated Show resolved Hide resolved
__tests__/utils.test.ts Outdated Show resolved Hide resolved
__tests__/utils.test.ts Outdated Show resolved Hide resolved
__tests__/Fuzzy.test.ts Outdated Show resolved Hide resolved
@KemingHe
Copy link
Contributor Author

Thanks @KemingHe for the PR ✨

I am thinking should we just add coverage instead of benchmarks ? wdyt ? This one I mean. - https://github.com/davelosert/vitest-coverage-report-action

@ad1992

  • Removed all benchmark test and script in 9bfc05c.
  • Impletemented coverage reporting in 9bfc05c, see screenshot below:
Screenshot 2024-07-28 at 3 16 15 PM

@KemingHe
Copy link
Contributor Author

  • 100% coverage in global Lines, Statements, Functions, and Branches.

Implemented testing for both cache hit and cache miss cases.

done #10
@KemingHe KemingHe changed the title test(__tests__ dir): added vitest and benchmarking to workflow test(__tests__ dir): added vitest and coverage reporting GitHub Actions workflow Jul 29, 2024
@KemingHe KemingHe mentioned this pull request Jul 29, 2024
Copy link
Owner

@ad1992 ad1992 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great @KemingHe!
Let's remove caching related stuff from this PR as discussed in the issue

@@ -0,0 +1,39 @@
# ./.github/workflows/node-ci.yml
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's rename this file to test-coverage-pr.yml

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import Fuzzy from '../src/Fuzzy';

// Fuzzy class test suite.
describe('Fuzzy', () => {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
describe('Fuzzy', () => {
describe('Test Fuzzy', () => {

Let's add a Test in each describe block just to make it more consistent

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

package.json Show resolved Hide resolved
# ./.github/workflows/node-ci.yml
#
# Auto CI/CD for Node.js projects using GitHub Actions.
name: Node CI
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
name: Node CI
name: Test Coverage Pull Request

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

workflow_dispatch:

jobs:
yarn-vitest:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
yarn-vitest:
coverage:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


jobs:
yarn-vitest:
name: Yarn Vitest
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
name: Yarn Vitest
name: Test Coverage

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KemingHe
Copy link
Contributor Author

Looks great @KemingHe! Let's remove caching related stuff from this PR as discussed in the issue

@ad1992

  • Hard reset to only testing in e3264f8.

@KemingHe
Copy link
Contributor Author

KemingHe commented Jul 29, 2024

Also @ad1992 , since >90% of our conversation has been about naming preferences, I propose that you provide the exact software specifications in ALL future instances BEFORE I start coding.

I can understand that this project is your world and everything and it must be perfect. However, I'm also just an unpaid volunteer and doing this out of my own free will.

Warning

If the micro-management continues, this will be my last commit to the project.

@ad1992
Copy link
Owner

ad1992 commented Jul 29, 2024

Also @ad1992 , since >90% of our conversation has been about naming preferences, I propose that you provide the exact software specifications in ALL future instances BEFORE I start coding.

I can understand that this project is your world and everything and it must be perfect. However, I'm also just an unpaid volunteer and doing this out of my own free will.

Warning

If the micro-management continues, this will be my last commit to the project.

Also @ad1992 , since >90% of our conversation has been about naming preferences, I propose that you provide the exact software specifications in ALL future instances BEFORE I start coding.

I can understand that this project is your world and everything and it must be perfect. However, I'm also just an unpaid volunteer and doing this out of my own free will.

Warning

If the micro-management continues, this will be my last commit to the project.

Thank you for sharing your concerns. I understand that discussing naming preferences can be annoying, but they are important to ensure consistency, clarity, and maintenance. However not all naming changes are necessary, some are good to have, so feel free to comment where you have a different opinion and we can learn from each other.

As mentioned in Readme, this library is at a very early stage and it will take me some time to bring everything in the documentation.

I acknowledge and appreciate the voluntary effort you are putting into this project and I am a volunteer as well so I hope we both have the same goals to improve the library and at the same time keep it maintainable as well.

From my side, I will try to add details and document as much as possible but as I mentioned it will take me some time as well, until then I will jump in and take care of those undocumented changes as well.

I hope we can continue working together smoothly and improve the library :).
Thanks again for your contributions so far ✨

Copy link
Owner

@ad1992 ad1992 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have removed the caching-related code and tests
Rest all looks good @KemingHe

For some reason, the types for globals aren't working even though specified in tsconfig. Will fix it in separate PR.

package.json Show resolved Hide resolved
@ad1992 ad1992 merged commit c98e6aa into ad1992:main Jul 29, 2024
3 checks passed
@KemingHe KemingHe deleted the test-addunittests-keminghe branch July 29, 2024 18:26
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.

2 participants