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(address.city): use string argument #683

Closed
wants to merge 15 commits into from
Closed

fix(address.city): use string argument #683

wants to merge 15 commits into from

Conversation

xDivisionByZerox
Copy link
Member

Fixes #682

@xDivisionByZerox xDivisionByZerox requested a review from a team as a code owner March 26, 2022 13:49
@xDivisionByZerox
Copy link
Member Author

Should the JSDOCs have a @see faker.fake line in it, since it's used to parse the formats?

@xDivisionByZerox
Copy link
Member Author

xDivisionByZerox commented Mar 26, 2022

Should I add tests for address.city since there are non so far. Or should this be done in a separate PR?

@codecov
Copy link

codecov bot commented Mar 26, 2022

Codecov Report

Merging #683 (c486c6f) into main (b41c662) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head c486c6f differs from pull request most recent head 313b8b1. Consider uploading reports for the commit 313b8b1 to get more accurate results

@@           Coverage Diff           @@
##             main     #683   +/-   ##
=======================================
  Coverage   99.42%   99.42%           
=======================================
  Files        1958     1958           
  Lines      210862   210879   +17     
  Branches      919      924    +5     
=======================================
+ Hits       209649   209666   +17     
  Misses       1156     1156           
  Partials       57       57           
Impacted Files Coverage Δ
src/address.ts 99.81% <100.00%> (+<0.01%) ⬆️

@ejcheng ejcheng added c: bug Something isn't working p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug labels Mar 26, 2022
@ejcheng ejcheng added this to the v6.1 - First bugfixes milestone Mar 26, 2022
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.

What is the added benefit of using address.prefix('format') to just using faker.fake('format')?

@ST-DDT
Copy link
Member

ST-DDT commented Mar 26, 2022

Should I add tests for address.city since there are non so far. Or should this be done in a separate PR?

Tests would be useful.

@ST-DDT ST-DDT added the needs test More tests are needed label Mar 28, 2022
Shinigami92
Shinigami92 previously approved these changes Mar 28, 2022
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.

Did I miss the test for city(number) and city(string)`?

@xDivisionByZerox
Copy link
Member Author

Did I miss the test for city(number) and city(string)`?

I didn't miss them, they are just not as trivial as checking if the return value exists in a predefined array^^

I'm still working on this from time to time. Should I convert this to a draft and reopen it when I'm done with the tests?

@ST-DDT ST-DDT marked this pull request as draft March 31, 2022 20:16
@xDivisionByZerox xDivisionByZerox removed the request for review from a team March 31, 2022 20:43
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.

Could you please extract the tests for the other address methods into a separate PR?

@xDivisionByZerox
Copy link
Member Author

So why do the helpers tests fail now?

@Shinigami92
Copy link
Member

So why do the helpers tests fail now?

Seems like some seeded values need to be updated

@ST-DDT ST-DDT added the needs rebase There is a merge conflict label Apr 4, 2022
@ST-DDT ST-DDT removed the needs rebase There is a merge conflict label Apr 8, 2022
@xDivisionByZerox
Copy link
Member Author

Yo, could it be that the dates generated by us break the test if they run on machines in different timezones? All tests pass on my end but fail here. The logs show expected date values 2012-02-01T23:00:00.000Z instead of 2012-02-02T00:00:00.000Z.
My timezone is UTC+1 without daylight saving times and UTC+2 with daylight saving times included.

@xDivisionByZerox
Copy link
Member Author

Nvm we expect any date in helpers.spec. But why can they fail in this case?

@xDivisionByZerox xDivisionByZerox removed the needs test More tests are needed label Apr 25, 2022
@xDivisionByZerox
Copy link
Member Author

I don't get this test

@ST-DDT
Copy link
Member

ST-DDT commented Apr 25, 2022

@xDivisionByZerox I reran the pipeline and now its fixed. Looks like a temporarily codecov server issue. All green now.

ST-DDT
ST-DDT previously approved these changes Apr 25, 2022
@xDivisionByZerox
Copy link
Member Author

xDivisionByZerox commented Apr 26, 2022

@Shinigami92 can you have a look at the error in address.nearbyGPSCoordinate (since you wrote the test for it)? I don't get how changing a depreciation message leads to this test failing.

@Shinigami92
Copy link
Member

Shinigami92 commented Apr 26, 2022

@Shinigami92 can you have a look at the error in address.nearbyGPSCoordinate (since you wrote the test for it)? I don't get how changing a depreciation message leads to this test failing.

Yeah I see, the seed for the execution of the nearbyGPSCoordinate check changed due to that the execution order of the tests changed because you added the city test above it.
It is not your fault! The altered seed just now shows that we have an expectation issue for the nearbyGPSCoordinate check.

@ST-DDT and I working on it now and try to solve it. => #876

@Shinigami92 Shinigami92 added the needs rebase There is a merge conflict label May 23, 2022
@ST-DDT
Copy link
Member

ST-DDT commented Jun 8, 2022

Superseded by #948

@ST-DDT ST-DDT closed this Jun 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: bug Something isn't working needs rebase There is a merge conflict p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

address.city string parameter ignored
4 participants