Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

test(animal): unroll animal test loop #3198

Merged
merged 6 commits into from
Oct 26, 2024

Conversation

matthewmayer
Copy link
Contributor

@matthewmayer matthewmayer commented Oct 19, 2024

Unroll the test loop for animal module

Alternative

@matthewmayer matthewmayer requested a review from a team as a code owner October 19, 2024 13:20
Copy link

netlify bot commented Oct 19, 2024

Deploy Preview for fakerjs ready!

Name Link
🔨 Latest commit 4ae6e0e
🔍 Latest deploy log https://app.netlify.com/sites/fakerjs/deploys/671cf480c8ee4500087c1fc9
😎 Deploy Preview https://deploy-preview-3198.fakerjs.dev
📱 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.

Copy link

codecov bot commented Oct 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.96%. Comparing base (975098d) to head (4ae6e0e).
Report is 1 commits behind head on next.

Additional details and impacted files
@@           Coverage Diff           @@
##             next    #3198   +/-   ##
=======================================
  Coverage   99.96%   99.96%           
=======================================
  Files        2804     2804           
  Lines      216974   216974           
  Branches      961      964    +3     
=======================================
+ Hits       216900   216904    +4     
+ Misses         74       70    -4     

see 1 file with indirect coverage changes

@ST-DDT ST-DDT added p: 1-normal Nothing urgent c: test m: animal Something is referring to the animal module labels Oct 19, 2024
@ST-DDT ST-DDT added this to the vAnytime milestone Oct 19, 2024
ST-DDT
ST-DDT previously approved these changes Oct 19, 2024
@ST-DDT ST-DDT requested review from a team October 19, 2024 18:33
Copy link
Contributor

@suyashgulati suyashgulati left a comment

Choose a reason for hiding this comment

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

This way makes the tests more verbose.

@matthewmayer
Copy link
Contributor Author

matthewmayer commented Oct 20, 2024

It's deliberately more verbose. I feel that test code shouldn't be too "clever".

I would argue if the tests were in this format it would have been easier to see what had gone wrong when copy pasting the new test for petName. The compiler / autocomplete would guide you that you needed pet_name not petName for the definition.

@ST-DDT ST-DDT added the s: needs decision Needs team/maintainer decision label Oct 23, 2024
Copy link
Member

@xDivisionByZerox xDivisionByZerox 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 to agree with @matthewmayer here. Test code is not supposed to be clever. Thinks like the DRY principle should not be applied to make the intend of the test more clear.

In the worst case test case isolation might even break by "clever" helper functions, since these functions are often not tested themself. While this is not the case here, I like to bring awareness of the posibilities for these situations.

Copy link
Member

@ST-DDT ST-DDT left a comment

Choose a reason for hiding this comment

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

It is a for loop. I wouldn't call it clever by any means.
A for loop enforces a basic level of consistency across all methods, that a copy paste approach does not.

Ultimately, it doesn't really matter to me.
So I'm fine either way.

@ST-DDT ST-DDT added s: accepted Accepted feature / Confirmed bug and removed s: needs decision Needs team/maintainer decision labels Oct 26, 2024
@ST-DDT ST-DDT enabled auto-merge (squash) October 26, 2024 13:54
@ST-DDT ST-DDT merged commit d57899f into faker-js:next Oct 26, 2024
23 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
c: test m: animal Something is referring to the animal module p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants