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(system): add valid 0 for cron #1787

Merged
merged 5 commits into from Jan 29, 2023
Merged

test(system): add valid 0 for cron #1787

merged 5 commits into from Jan 29, 2023

Conversation

Shinigami92
Copy link
Member

While working on

it reports an failure for https://github.com/faker-js/faker/actions/runs/4025222101/jobs/6918173202#step:7:302

but 0 is a valid digit for cron

@Shinigami92 Shinigami92 requested a review from a team as a code owner January 27, 2023 16:46
@Shinigami92 Shinigami92 self-assigned this Jan 27, 2023
@Shinigami92 Shinigami92 requested a review from a team January 27, 2023 16:47
@codecov
Copy link

codecov bot commented Jan 27, 2023

Codecov Report

Merging #1787 (9e2a514) into next (7bc4a09) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files
@@           Coverage Diff           @@
##             next    #1787   +/-   ##
=======================================
  Coverage   99.65%   99.65%           
=======================================
  Files        2340     2340           
  Lines      242622   242622           
  Branches     1113     1113           
=======================================
  Hits       241786   241786           
  Misses        815      815           
  Partials       21       21           

import-brain
import-brain previously approved these changes Jan 27, 2023
ST-DDT
ST-DDT previously requested changes Jan 27, 2023
test/system.spec.ts Outdated Show resolved Hide resolved
@xDivisionByZerox xDivisionByZerox added p: 1-normal Nothing urgent m: system Something is referring to the system module labels Jan 28, 2023
@Shinigami92
Copy link
Member Author

This PR targeted the wrong goal and was meant to fix the shuffled tests
Now we want to change the whole implementation of the test itself
Therefore it's up to be grepped by someone who wants to fix it

@xDivisionByZerox
Copy link
Member

I'll provide a follow up PR which addresses this test reimplementation.

@xDivisionByZerox
Copy link
Member

After checking the source I noticed that the fix for the test was totally valid.
includeNonStandard is misleading in that way that it doesn't guarantee that a non standard expression is returned. Thats why we simply have to check whether a number/astrics (standard) or an @ (non standard) is returned.

The title should probably simply be adjusted to state 'should be able to return non-standard cron expressions'.

@Shinigami92 Shinigami92 dismissed ST-DDT’s stale review January 29, 2023 14:30

please provide a new review

@xDivisionByZerox xDivisionByZerox requested review from a team January 29, 2023 14:36
@ST-DDT ST-DDT enabled auto-merge (squash) January 29, 2023 16:40
@ST-DDT ST-DDT merged commit 64f480d into next Jan 29, 2023
@ST-DDT ST-DDT deleted the test-add-valid-0-for-cron branch January 29, 2023 16:58
matthewmayer pushed a commit to matthewmayer/faker that referenced this pull request Feb 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: test m: system Something is referring to the system module p: 1-normal Nothing urgent
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants