nodejs / nodejs.org

The Node.js® Website
https://nodejs.org
MIT License
6.15k stars 6.21k forks source link

fix: added digit regex #6728

Closed TenzDelek closed 4 months ago

TenzDelek commented 4 months ago

the current behavior:

image

after adding digit regex:

image
vercel[bot] commented 4 months ago

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

Name Status Preview Updated (UTC)
nodejs-org ✅ Ready (Inspect) Visit Preview May 21, 2024 8:52am
AugustinMauroy commented 4 months ago

I think it would be good to have a unit test for this would you do it? That would make it possible once and for all to see if all the use cases are correct.

TenzDelek commented 4 months ago

I think it would be good to have a unit test for this would you do it? That would make it possible once and for all to see if all the use cases are correct.

I believe I don't have prior experience with writing unit test for now, but I would love to do it . If it possible for any one of the members to guide me it can be helpful.

AugustinMauroy commented 4 months ago

OK I can help you We have a docs General Guidelines for Unit Tests can help you

there are already a unit test file for util/stringUtils.ts it's util/__tests__/stringUtils.test.ts.

to add the test:

  1. import function
  2. each use case of the function must be covered by an it. For example : it("dashToCamelCase should work with number", () => {
  3. need to use expect to "expect" what test should test. I recommend you to read this doc https://jestjs.io/docs/expect

npm run test to run test suit. if everything pass you can commit ! also you can use npm run test:unit:watch to start watching file so each time you will save it's will re-run the task

TenzDelek commented 4 months ago

OK I can help you We have a docs General Guidelines for Unit Tests can help you

there are already a unit test file for util/stringUtils.ts it's util/__tests__/stringUtils.test.ts.

to add the test:

  1. import function
  2. each use case of the function must be covered by an it. For example : it("dashToCamelCase should work with number", () => {
  3. need to use expect to "expect" what test should test. I recommend you to read this doc https://jestjs.io/docs/expect

npm run test to run test suit. if everything pass you can commit ! also you can use npm run test:unit:watch to start watching file so each time you will save it's will re-run the task

Thanks a lot !! will go through the docs and check

TenzDelek commented 4 months ago

hello @AugustinMauroy the testcases are passing for dashtocamel. (can you check once whether the testcase are fine. i used some random values) thanks for the help.

also one thing to be note, during the test, I think one of the testcase (unrelated to this pr) is failing.

image
github-actions[bot] commented 4 months ago
Lighthouse Results URL Performance Accessibility Best Practices SEO Report
/en 🟠 84 🟢 100 🟢 100 🟢 91 🔗
/en/about 🟢 98 🟢 100 🟢 100 🟢 91 🔗
/en/about/previous-releases 🟢 98 🟢 100 🟢 100 🟢 92 🔗
/en/download 🟢 94 🟢 100 🟢 100 🟢 91 🔗
/en/blog 🟢 97 🟢 100 🟢 100 🟢 92 🔗
github-actions[bot] commented 4 months ago

Unit Test Coverage Report

Lines Statements Branches Functions
Coverage: 92%
90.51% (592/654) 76.08% (175/230) 93.79% (121/129)

Unit Test Report

Tests Skipped Failures Errors Time
131 0 :zzz: 0 :x: 0 :fire: 6.47s :stopwatch:
AugustinMauroy commented 4 months ago

@TenzDelek strange because here on CI it's pass

TenzDelek commented 4 months ago

@AugustinMauroy again thanks for the help, that's the one thing that I love about open source is that we can learn and explore new things. Happy learning 🙏🏌

AugustinMauroy commented 4 months ago

@AugustinMauroy again thanks for the help, that's the one thing that I love about open source is that we can learn and explore new things. Happy learning 🙏🏌

I try to do my best, but I'm not a teacher 😄.

bmuenzenmeyer commented 4 months ago

https://nodejs-org-git-fork-tenzdelek-fix-added-digit-regex-openjs.vercel.app/en/learn/manipulating-files/working-with-different-filesystems still looks broken to me

maybe we need a combo of both solutions 😄

ovflowd commented 4 months ago

As commented on the other PR, I'm closing this one in favour of @abizek's PR 🙇