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: refactored test-intl #8641

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@n0f3
Contributor

n0f3 commented Sep 18, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)
Description of change

Improved variables to be all const.
Enforced assert.strictEqual across all tests.
Modified haveIntl tests to check for int.

@mscdex mscdex added the intl label Sep 18, 2016

@Trott

Trott approved these changes Sep 18, 2016

LGTM if CI comes up OK

Show outdated Hide outdated test/parallel/test-intl.js
@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Sep 18, 2016

Member

@nodejs/intl (especially @srl295 I suppose) What's up with the undefined check on line 7? The setting might legitimately not exist at all? Or is that there for backwards compatibility? Or ChakraCore compatibility?

Member

Trott commented Sep 18, 2016

@nodejs/intl (especially @srl295 I suppose) What's up with the undefined check on line 7? The setting might legitimately not exist at all? Or is that there for backwards compatibility? Or ChakraCore compatibility?

@Trott

This comment has been minimized.

Show comment
Hide comment
@n0f3

This comment has been minimized.

Show comment
Hide comment
@n0f3

n0f3 Sep 18, 2016

Contributor

should I worry about test/arm-fanned failing? locally everything ran fine

Contributor

n0f3 commented Sep 18, 2016

should I worry about test/arm-fanned failing? locally everything ran fine

test: refactored test-intl
Improved variables to be all const.
Enforced assert.strictEqual across all tests.
Modified haveIntl tests to check for int.

fixed alignment
@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Sep 18, 2016

Member

No, no need to worry, those failing tests are known flaky tests on Raspberry Pi. They're even marked as flaky on armv6 but for whatever reason, test.py isn't picking up on that.

Member

Trott commented Sep 18, 2016

No, no need to worry, those failing tests are known flaky tests on Raspberry Pi. They're even marked as flaky on armv6 but for whatever reason, test.py isn't picking up on that.

@imyller

LGTM

@imyller

This comment has been minimized.

Show comment
Hide comment
@imyller

imyller Sep 19, 2016

Member

Ready for landing?

I'd assume @Trott that your comment on (enablei18n === undefined) is not reason to hold this PR?

Member

imyller commented Sep 19, 2016

Ready for landing?

I'd assume @Trott that your comment on (enablei18n === undefined) is not reason to hold this PR?

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Sep 19, 2016

Member

@imyller Yeah, my comment can be ignored. It can be addressed in a subsequent PR (or not at all, I suppose).

I would say this is not yet ready to land solely because it has not been open for 48 hours. But it LGTM.

Member

Trott commented Sep 19, 2016

@imyller Yeah, my comment can be ignored. It can be addressed in a subsequent PR (or not at all, I suppose).

I would say this is not yet ready to land solely because it has not been open for 48 hours. But it LGTM.

@imyller

This comment has been minimized.

Show comment
Hide comment
@imyller

imyller Sep 19, 2016

Member

@Trott Oh, GH rounds the visualized time to "2 days", but you're right: still ~8 hours to full 48 hour window for this one.

I'll start a new CI run to prepare and take this to landing tomorrow if there are no last minute objections.

Member

imyller commented Sep 19, 2016

@Trott Oh, GH rounds the visualized time to "2 days", but you're right: still ~8 hours to full 48 hour window for this one.

I'll start a new CI run to prepare and take this to landing tomorrow if there are no last minute objections.

@imyller imyller self-assigned this Sep 19, 2016

@imyller

This comment has been minimized.

Show comment
Hide comment

imyller added a commit to imyller/node that referenced this pull request Sep 20, 2016

test: cleanup test-intl.js
Improved variables to be all const.
Enforced assert.strictEqual across all tests.
Modified haveIntl tests to check for int.
Fixed alignment.

PR-URL: nodejs#8641
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
@imyller

This comment has been minimized.

Show comment
Hide comment
@imyller

imyller Sep 20, 2016

Member

landed in 2eb3fa1

Thank you for your contribution, @n0f3

Member

imyller commented Sep 20, 2016

landed in 2eb3fa1

Thank you for your contribution, @n0f3

@imyller imyller closed this Sep 20, 2016

@imyller imyller removed their assignment Sep 20, 2016

Fishrock123 added a commit that referenced this pull request Oct 11, 2016

test: cleanup test-intl.js
Improved variables to be all const.
Enforced assert.strictEqual across all tests.
Modified haveIntl tests to check for int.
Fixed alignment.

PR-URL: #8641
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment