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

ci: add node version matrix #179

Merged
merged 6 commits into from
Jan 18, 2022
Merged

Conversation

MohdImran001
Copy link
Contributor

Fixes #159

@prisis please review

@netlify
Copy link

netlify bot commented Jan 16, 2022

✔️ Deploy Preview for vigilant-wescoff-04e480 ready!

🔨 Explore the source changes: f929c7c

🔍 Inspect the deploy log: https://app.netlify.com/sites/vigilant-wescoff-04e480/deploys/61e7378a80a6a2000892579f

😎 Browse the preview: https://deploy-preview-179--vigilant-wescoff-04e480.netlify.app

@Shinigami92 Shinigami92 self-requested a review January 16, 2022 17:24
@Shinigami92 Shinigami92 added the p: 3-urgent Fix and release ASAP label Jan 16, 2022
@Shinigami92 Shinigami92 marked this pull request as draft January 16, 2022 17:25
@prisis
Copy link
Member

prisis commented Jan 18, 2022

@MohdImran001 do you need help with it?

@MohdImran001
Copy link
Contributor Author

Sorry I was a bit busy , I will complete this today.

@MohdImran001 MohdImran001 changed the title chore: add node version matrix ci: add node version matrix Jan 18, 2022
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@Shinigami92 Shinigami92 marked this pull request as ready for review January 18, 2022 17:21
@Shinigami92
Copy link
Member

Shinigami92 commented Jan 18, 2022

We need to find out what the npm ERR! google-closure-compiler-linux not accessible from google-closure-compiler-error is
And why it fails with one test in windows 🤔

Edit:

image 👀

@Shinigami92
Copy link
Member

@MohdImran001 Please remove the dependency "node-minify": "*", and then run rm -Rf node_modules package-lock.json && npm install
Then commit both files: package.json and packge-lock.json

@Shinigami92
Copy link
Member

Nice, now node 14 and 12 is working 👍
But we need to find out why the one test in windows is failing

@Shinigami92
Copy link
Member

It's actually this test:

faker/test/finance.unit.js

Lines 283 to 299 in d72482b

it('should return the number formatted on the current locale', function () {
const number = 6000,
decimalPlaces = 2;
const expected = number.toLocaleString(undefined, {
minimumFractionDigits: decimalPlaces,
});
const amount = faker.finance.amount(
number,
number,
decimalPlaces,
undefined,
true
);
assert.strictEqual(amount, expected);
});

@ST-DDT
Copy link
Member

ST-DDT commented Jan 18, 2022

I am unable to reproduce the test error on both my win11 and win10 computers.

@Shinigami92
Copy link
Member

I hacked around it by conditionally disable the test on win32
At least it is better for now to have the other machines running all the tests, so in total we have 4x22k+ more tests running on node 12, 14 and mac and win
I will create an issue so we remember to fix this test later

@Shinigami92 Shinigami92 merged commit 80e8b3d into faker-js:main Jan 18, 2022
pkuczynski pushed a commit to pkuczynski/faker that referenced this pull request Jan 21, 2022
Co-authored-by: Shinigami92 <chrissi92@hotmail.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p: 3-urgent Fix and release ASAP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ci: add matrix of supported node versions
6 participants