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

fix(image): dataUri is not random #2316

Merged
merged 23 commits into from
Sep 17, 2023
Merged

fix(image): dataUri is not random #2316

merged 23 commits into from
Sep 17, 2023

Conversation

OmkarPednekar
Copy link
Contributor

@OmkarPednekar OmkarPednekar commented Aug 14, 2023

If color option is not provided the code will use a randomly generated color code as default color
the fix(module):faker.image.dataUri commit is the final one. where I have used faker.color.rgb in the previous commit I was using a for loop to generate a hex code. @ST-DDT suggested me to use the inbuilt functions so that we have reproducible values.

fixes #2307

@codecov
Copy link

codecov bot commented Aug 14, 2023

Codecov Report

Merging #2316 (74925a0) into next (82b779d) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #2316      +/-   ##
==========================================
- Coverage   99.61%   99.60%   -0.01%     
==========================================
  Files        2793     2793              
  Lines      251586   251586              
  Branches     1100     1096       -4     
==========================================
- Hits       250611   250597      -14     
- Misses        948      962      +14     
  Partials       27       27              
Files Changed Coverage Δ
src/modules/image/index.ts 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

@matthewmayer matthewmayer added p: 1-normal Nothing urgent c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs m: image Something is referring to the image module labels Aug 14, 2023
@matthewmayer matthewmayer changed the title fix(module): faker.image.dataUri is not random #2307 fix(module): faker.image.dataUri is not random Aug 14, 2023
@ST-DDT
Copy link
Member

ST-DDT commented Aug 14, 2023

I just noticed, that dataUri doesn't have seeded tests, so I created a PR for that.

Blocked by:

@ST-DDT ST-DDT added the s: on hold Blocked by something or frozen to avoid conflicts label Aug 14, 2023
@ST-DDT ST-DDT changed the title fix(module): faker.image.dataUri is not random fix(image): dataUri is not random Aug 14, 2023
@OmkarPednekar
Copy link
Contributor Author

is this okay?
image

@ST-DDT
Copy link
Member

ST-DDT commented Aug 14, 2023

is this okay? image

I would probably phrase it like this:

The background color of the image. Must be a color supported by svg. Defaults to a random color.

@OmkarPednekar
Copy link
Contributor Author

Okay

src/modules/image/index.ts Outdated Show resolved Hide resolved
@OmkarPednekar
Copy link
Contributor Author

Please Check

src/modules/image/index.ts Outdated Show resolved Hide resolved
@OmkarPednekar
Copy link
Contributor Author

Made the changes in @param as well

@OmkarPednekar
Copy link
Contributor Author

OmkarPednekar commented Aug 14, 2023

image
Updated the Docs

src/modules/image/index.ts Outdated Show resolved Hide resolved
src/modules/image/index.ts Outdated Show resolved Hide resolved
test/modules/image.spec.ts Outdated Show resolved Hide resolved
test/modules/image.spec.ts Outdated Show resolved Hide resolved
@ST-DDT ST-DDT requested review from import-brain, xDivisionByZerox and a team September 15, 2023 19:50
Shinigami92
Shinigami92 previously approved these changes Sep 15, 2023
@ST-DDT ST-DDT dismissed stale reviews from Shinigami92 and themself via 94053fb September 15, 2023 20:13
ST-DDT
ST-DDT previously approved these changes Sep 15, 2023
@ST-DDT ST-DDT requested review from Shinigami92 and a team September 15, 2023 20:14
Shinigami92
Shinigami92 previously approved these changes Sep 15, 2023
@Shinigami92 Shinigami92 self-requested a review September 15, 2023 20:17
@Shinigami92
Copy link
Member

👀

@Shinigami92 Shinigami92 dismissed their stale review September 15, 2023 20:17

test failures

@OmkarPednekar
Copy link
Contributor Author

Why are these tests failing?

@ST-DDT
Copy link
Member

ST-DDT commented Sep 15, 2023

I fixed the random seeded tests using test snapshots.

@ST-DDT
Copy link
Member

ST-DDT commented Sep 15, 2023

Why are these tests failing?

Because the randomized tests used snapshots which would work fine if the result is static, but this PR changes them to be random thus failing the tests. I fixed them by not using the static snapshots and instead check via validatorjs's isDataUri.

@OmkarPednekar
Copy link
Contributor Author

Why are these tests failing?

Because the randomized tests used snapshots which would work fine if the result is static, but this PR changes them to be random thus failing the tests. I fixed them by not using the static snapshots and instead check via validatorjs's isDataUri.

But I had removed those test cases and changed them to match the changes

@ST-DDT
Copy link
Member

ST-DDT commented Sep 15, 2023

But I had removed those test cases and changed them to match the changes

#2273 added new ones.

@OmkarPednekar
Copy link
Contributor Author

But I had removed those test cases and changed them to match the changes

#2273 added new ones.

Oh I seee so how can we fix this failing tests?

@ST-DDT
Copy link
Member

ST-DDT commented Sep 15, 2023

Oh I seee so how can we fix this failing tests?

I already removed/fixed them.

@OmkarPednekar
Copy link
Contributor Author

Oh I seee so how can we fix this failing tests?

I already removed/fixed them.

No I meant the merge checks

@ST-DDT
Copy link
Member

ST-DDT commented Sep 15, 2023

No I meant the merge checks

Not sure what you are referring to.

grafik

@OmkarPednekar
Copy link
Contributor Author

No I meant the merge checks

Not sure what you are referring to.

grafik

Oh whatt just a moment ago they were appearing failed lol

@ST-DDT ST-DDT merged commit a7d25fa into faker-js:next Sep 17, 2023
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs m: image Something is referring to the image module p: 1-normal Nothing urgent
Projects
None yet
Development

Successfully merging this pull request may close these issues.

faker.image.dataUri is not random
6 participants