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: migrate datatype to test snapshots #875

Merged
merged 30 commits into from
Jun 18, 2022
Merged

Conversation

ST-DDT
Copy link
Member

@ST-DDT ST-DDT commented Apr 26, 2022

We should migrate from our old inline seeded tests to using test snapshots, as these are way easier to update.
I plan to update the other modules as well, but this one is the first in case something needs to be changed.

@ST-DDT ST-DDT added p: 1-normal Nothing urgent c: test labels Apr 26, 2022
@ST-DDT ST-DDT added this to the v6.3 - Next Minor milestone Apr 26, 2022
@ST-DDT ST-DDT self-assigned this Apr 26, 2022
@ST-DDT ST-DDT requested a review from a team as a code owner April 26, 2022 12:10
@ST-DDT ST-DDT requested a review from a team April 26, 2022 12:10
@codecov
Copy link

codecov bot commented Apr 26, 2022

Codecov Report

Merging #875 (922b287) into main (ab2ed23) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #875      +/-   ##
==========================================
- Coverage   99.64%   99.64%   -0.01%     
==========================================
  Files        2146     2146              
  Lines      230380   230380              
  Branches      980      979       -1     
==========================================
- Hits       229571   229569       -2     
- Misses        788      790       +2     
  Partials       21       21              
Impacted Files Coverage Δ
src/modules/internet/user-agent.ts 86.37% <0.00%> (-0.58%) ⬇️

@xDivisionByZerox
Copy link
Member

I'm sorry but in what world is this easier to maintain? By changing any label in the entire test chain, you have an invalid expectation key.

Also since every snapshot value is a string, prettier can't apply the style guide correctly. Or am I missing something?

Can you please elaborate on how this helps maintainability wise?

@Shinigami92
Copy link
Member

Shinigami92 commented Apr 26, 2022

Snapshots can be updated by just using --update

But yeah, I was not aware that it just uses strings and therefore we loose the types 🤔
But maybe vitest uses some magic behind

@xDivisionByZerox
Copy link
Member

Ok, I didn't know there was such an option. Thought the migration was done by hand.

@Shinigami92
Copy link
Member

Ok, I didn't know there was such an option. Thought the migration was done by hand.

That's the whole point of using snapshots 😉

@Shinigami92
Copy link
Member

I would prefer to have all tests migrated in one PR so it is only one commit in main branch history. You could create commits test by test to make review easier.

@import-brain import-brain added the needs rebase There is a merge conflict label Jun 7, 2022
@ST-DDT ST-DDT removed the needs rebase There is a merge conflict label Jun 16, 2022
@ST-DDT
Copy link
Member Author

ST-DDT commented Jun 16, 2022

All converted. Ready for review.

import-brain
import-brain previously approved these changes Jun 16, 2022
Shinigami92
Shinigami92 previously approved these changes Jun 16, 2022
@Shinigami92 Shinigami92 added the s: accepted Accepted feature / Confirmed bug label Jun 16, 2022
import-brain
import-brain previously approved these changes Jun 17, 2022
@Shinigami92 Shinigami92 added the needs rebase There is a merge conflict label Jun 18, 2022
@ST-DDT ST-DDT dismissed stale reviews from import-brain and Shinigami92 via 532e988 June 18, 2022 15:03
@Shinigami92 Shinigami92 enabled auto-merge (squash) June 18, 2022 15:17
@Shinigami92 Shinigami92 removed the needs rebase There is a merge conflict label Jun 18, 2022
@Shinigami92 Shinigami92 merged commit 1898f28 into main Jun 18, 2022
@ST-DDT ST-DDT deleted the tests/snapshots/datatype branch June 18, 2022 17:04
Minozzzi pushed a commit to Minozzzi/faker that referenced this pull request Jul 19, 2022
@xDivisionByZerox xDivisionByZerox added the m: datatype Something is referring to the datatype module label Aug 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: test m: datatype Something is referring to the datatype 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.

4 participants