FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: disable test-tick-processor - aix and be ppc by mhdawson · Pull Request #3491 · nodejs/node · GitHub

/ node Public

test: disable test-tick-processor - aix and be ppc - #3491

Closed
mhdawson wants to merge 1 commit into
nodejs:masterfrom
mhdawson:tick
ConversationCommits (1)ChecksFiles changed
Closed

test: disable test-tick-processor - aix and be ppc#3491
mhdawson wants to merge 1 commit into
nodejs:masterfrom
mhdawson:tick

Conversation

mhdawson commented Oct 22, 2015

Copy link
Copy Markdown
Member

This test is already partially disabled for several platforms with
the comment that the required info is not provided at the C++ level.
I'm adding AIX as and PPC BE linux as they currently fall into
the same category. We are working to see if we can change that
in v8 but it will be non-trivial if is possible at all so I don't
want to leave the CI with failing tests until that point.

mhdawson added the test Issues and PRs related to the tests. label Oct 22, 2015

bnoordhuis commented Oct 22, 2015

Copy link
Copy Markdown
Member

LGTM if the CI is happy now. Maybe drop the superfluous parentheses.

Fishrock123 reviewed Oct 22, 2015
View reviewed changes
Comment thread test/parallel/test-tick-processor.js Outdated

Fishrock123 Oct 22, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The latter should be a helper.

Please also switch to using common.isWindows and common.isAix. :)

jasnell Oct 22, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

os.endianness() is already defined

mhdawson commented Oct 22, 2015

Copy link
Copy Markdown
Member Author

@Fishrock123 @jasnell updated to address comments.

mhdawson commented Oct 22, 2015

Copy link
Copy Markdown
Member Author

CI run https://ci.nodejs.org/job/node-test-pull-request/568/

jasnell commented Oct 23, 2015

Copy link
Copy Markdown
Member

LGTM. The CI run is borked, however, due to a bad npm update. Will have to rerun after that is addressed.

Trott commented Oct 23, 2015

Copy link
Copy Markdown
Member

LGTM if CI is happy. CI is unborked now, so here's a new CI run: https://ci.nodejs.org/job/node-test-pull-request/577/

EDIT: First green node-test-commit-plinux ever! 🎉

Trott reviewed Oct 23, 2015
View reviewed changes
Comment thread test/common.js

Trott Oct 23, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Nit that can totally be ignored: isLinuxPpcBe perhaps for consistency? (We don't do isFreeBSD or isSunOS for example.)

mhdawson Oct 23, 2015

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Agreed on consistency but since I just added the ones for FreeBSD and SunOS I'll change those instead

mhdawson commented Oct 23, 2015

Copy link
Copy Markdown
Member Author

Updated to improve captilization for SunOS and FreeBSD

mhdawson commented Oct 23, 2015

Copy link
Copy Markdown
Member Author

Another CI run after change https://ci.nodejs.org/job/node-test-pull-request/578/

jasnell commented Oct 23, 2015

Copy link
Copy Markdown
Member

Nice to see the ppc CI all green. LGTM

test: disable test-tick-processor - aix and be ppc
3a6fd02 
This test is already partially disabled for several platforms with
the comment that the required info is not provided at the C++ level.
I'm adding AIX as and PPC BE linux as they currently fall into
the same category.  We are working to see if we can change that
in v8 but it will be non-trivial if is possible at all so I don't
want to leave the CI with failing tests until that point.

mhdawson commented Oct 23, 2015

Copy link
Copy Markdown
Member Author

Squashed down to 1 commit

mhdawson added a commit that referenced this pull request Oct 23, 2015
test: disable test-tick-processor - aix and be ppc
5f3fb1c 
This test is already partially disabled for several platforms with
the comment that the required info is not provided at the C++ level.
I'm adding AIX as and PPC BE linux as they currently fall into
the same category.  We are working to see if we can change that
in v8 but it will be non-trivial if is possible at all so I don't
want to leave the CI with failing tests until that point.

PR-URL: #3491
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>

mhdawson commented Oct 23, 2015

Copy link
Copy Markdown
Member Author

Landed as 5f3fb1c

mhdawson closed this Oct 23, 2015
mhdawson mentioned this pull request Oct 23, 2015
Closed
mhdawson added a commit to ibmruntimes/node that referenced this pull request Oct 23, 2015
test: disable test-tick-processor - aix and be ppc
2c35210 
This test is already partially disabled for several platforms with
the comment that the required info is not provided at the C++ level.
I'm adding AIX as and PPC BE linux as they currently fall into
the same category.  We are working to see if we can change that
in v8 but it will be non-trivial if is possible at all so I don't
want to leave the CI with failing tests until that point.

PR-URL: nodejs/node#3491
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
mhdawson added a commit that referenced this pull request Oct 26, 2015
test: disable test-tick-processor - aix and be ppc
bc2a80b 
This test is already partially disabled for several platforms with
the comment that the required info is not provided at the C++ level.
I'm adding AIX as and PPC BE linux as they currently fall into
the same category.  We are working to see if we can change that
in v8 but it will be non-trivial if is possible at all so I don't
want to leave the CI with failing tests until that point.

PR-URL: #3491
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>

jasnell commented Oct 26, 2015

Copy link
Copy Markdown
Member

Landed in v4.x-staging in bc2a80b

mhdawson added a commit that referenced this pull request Oct 26, 2015
test: disable test-tick-processor - aix and be ppc
08da5c2 
This test is already partially disabled for several platforms with
the comment that the required info is not provided at the C++ level.
I'm adding AIX as and PPC BE linux as they currently fall into
the same category.  We are working to see if we can change that
in v8 but it will be non-trivial if is possible at all so I don't
want to leave the CI with failing tests until that point.

PR-URL: #3491
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
This was referenced Oct 27, 2015
Merged
Closed
mhdawson added a commit that referenced this pull request Oct 29, 2015
test: disable test-tick-processor - aix and be ppc
d409ac4 
This test is already partially disabled for several platforms with
the comment that the required info is not provided at the C++ level.
I'm adding AIX as and PPC BE linux as they currently fall into
the same category.  We are working to see if we can change that
in v8 but it will be non-trivial if is possible at all so I don't
want to leave the CI with failing tests until that point.

PR-URL: #3491
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
mhdawson deleted the tick branch May 9, 2016 22:39
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL