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

test: skip the unsupported test cases for IBM i by dmabupt · Pull Request #30819 · nodejs/node · GitHub

/ node Public

test: skip the unsupported test cases for IBM i - #30819

Closed
dmabupt wants to merge 2 commits into
nodejs:masterfrom
dmabupt:ibmi_skipcase
ConversationCommits (2)ChecksFiles changed
Closed

test: skip the unsupported test cases for IBM i#30819
dmabupt wants to merge 2 commits into
nodejs:masterfrom
dmabupt:ibmi_skipcase

Conversation

dmabupt commented Dec 6, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

This is a following PR of #30714.

The cases marked with // TODO need more investigations to identify if it is a system limit or can be resolved by code change. And I am working on it.

The other cases are identified that IBM i does not support them.

What we need to do is to indentify (and/or resolve) all the // TODO items.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

nodejs-github-bot added async_hooks Issues and PRs related to the async hooks subsystem. test Issues and PRs related to the tests. labels Dec 6, 2019
addaleax approved these changes Dec 6, 2019
View reviewed changes
Comment thread test/parallel/test-c-ares.js Outdated
Comment thread test/parallel/test-child-process-spawnsync-validation-errors.js Outdated
Comment thread test/parallel/test-child-process-uid-gid.js Outdated
cjihrig reviewed Dec 6, 2019
View reviewed changes
Comment thread test/async-hooks/test-fseventwrap.js Outdated
Comment thread test/parallel/test-c-ares.js Outdated
Comment thread test/parallel/test-child-process-fork-net-server.js Outdated
Comment thread test/parallel/test-cli-node-options.js Outdated
Comment thread test/parallel/test-fs-watch-close-when-destroyed.js Outdated
Comment thread test/parallel/test-http-writable-true-after-close.js Outdated
bnoordhuis reviewed Dec 6, 2019
View reviewed changes

bnoordhuis left a comment

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

High-level comment: you should decide on spelling it either as IBMi or IBM i but not both. :-)

Comment thread test/async-hooks/test-fseventwrap.js Outdated
Comment thread test/parallel/test-child-process-fork-net-server.js Outdated
Comment thread test/parallel/test-http-writable-true-after-close.js Outdated
Comment thread test/parallel/test-module-loading-error.js Outdated
Comment thread test/parallel/test-os.js Outdated
Comment thread test/sequential/test-inspector-contexts.js Outdated

sam-github commented Dec 6, 2019

Copy link
Copy Markdown
Contributor

@dmabupt This is a Draft, so you should say what needs to be done to make it complete.

I assume that all the // TODO comments for example you intend to investigate and resolve them before requesting merge?

sam-github added the wip Issues and PRs that are still a work in progress. label Dec 6, 2019

dmabupt commented Dec 7, 2019

Copy link
Copy Markdown
Contributor Author

@dmabupt This is a Draft, so you should say what needs to be done to make it complete.

I assume that all the // TODO comments for example you intend to investigate and resolve them before requesting merge?

Yes, I am investigating the // TODO cases. And I just added the descriptions of what needs to be done.

richardlau commented Dec 7, 2019

Copy link
Copy Markdown
Member

A suggestion regarding all of the common.skip()'s with TODO's -- If these are temporary with the intention to be investiagted and fixed later then another way might be to use the status files, e.g. (example with AIX)

node/test/sequential/sequential.status

Lines 21 to 23 in 4ec02d5

[$system==aix]
# https://github.com/nodejs/node/pull/29054
test-buffer-creation-regression: SKIP

This would list all of them in one place (for each test bucket) and would mean the affected test files wouldn't need to be changed here. Would need IBMi detection to be added to

node/tools/utils.py

Lines 46 to 69 in 4ec02d5

def GuessOS():
id = platform.system()
if id == 'Linux':
return 'linux'
elif id == 'Darwin':
return 'macos'
elif id.find('CYGWIN') >= 0:
return 'cygwin'
elif id == 'Windows' or id == 'Microsoft':
# On Windows Vista platform.system() can return 'Microsoft' with some
# versions of Python, see http://bugs.python.org/issue1082
return 'win32'
elif id == 'FreeBSD':
return 'freebsd'
elif id == 'OpenBSD':
return 'openbsd'
elif id == 'SunOS':
return 'solaris'
elif id == 'NetBSD':
return 'netbsd'
elif id == 'AIX':
return 'aix'
else:
return None

dmabupt commented Dec 7, 2019

Copy link
Copy Markdown
Contributor Author

A suggestion regarding all of the common.skip()'s with TODO's -- If these are temporary with the intention to be investiagted and fixed later then another way might be to use the status files, e.g. (example with AIX)

node/test/sequential/sequential.status

Lines 21 to 23 in 4ec02d5

[$system==aix]
# https://github.com/nodejs/node/pull/29054
test-buffer-creation-regression: SKIP

This would list all of them in one place (for each test bucket) and would mean the affected test files wouldn't need to be changed here. Would need IBMi detection to be added to

node/tools/utils.py

Lines 46 to 69 in 4ec02d5

def GuessOS():
id = platform.system()
if id == 'Linux':
return 'linux'
elif id == 'Darwin':
return 'macos'
elif id.find('CYGWIN') >= 0:
return 'cygwin'
elif id == 'Windows' or id == 'Microsoft':
# On Windows Vista platform.system() can return 'Microsoft' with some
# versions of Python, see http://bugs.python.org/issue1082
return 'win32'
elif id == 'FreeBSD':
return 'freebsd'
elif id == 'OpenBSD':
return 'openbsd'
elif id == 'SunOS':
return 'solaris'
elif id == 'NetBSD':
return 'netbsd'
elif id == 'AIX':
return 'aix'
else:
return None

GuessOS()

That is great.
The only concern is that GuessOS() returns either aix or os400. If let it return the real OS name, will it affect other modules reusing the aix code?

richardlau commented Dec 7, 2019

Copy link
Copy Markdown
Member

GuessOS()

That is great.
The only concern is that GuessOS() returns either aix or os400. If let it return the real OS name, will it affect other modules reusing the aix code?

GuessOS() in tools/utils.py is only used to match things in the test *.status files. I think for IBMi it should return os400 (or ibmi, your choice). If there are existing tests that have entries in the status file for [$system==aix] that should also apply to IBMi we should just duplicate them for [$system==os400] (or [$system==ibmi]).

BridgeAR reviewed Dec 8, 2019
View reviewed changes
Comment thread test/parallel/test-os.js Outdated
dmabupt commented Dec 8, 2019
View reviewed changes
Comment thread tools/utils.py Outdated
dmabupt marked this pull request as ready for review December 9, 2019 05:28
deps: V8: cherry-pick d406bfd64653
75c6499 
Original commit message:

    [base] Fix the return of ClockNow on IBMi

    The API thread_cputime() is only defined but not yet implemented on IBMi.

    Change-Id: I8ea7ff724e749f537b54e75a00d718500807ca8a
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1957831
    Reviewed-by: Junliang Yan <jyan@ca.ibm.com>
    Reviewed-by: Clemens Backes <clemensb@chromium.org>
    Commit-Queue: Milad Farazmand <miladfar@ca.ibm.com>
    Cr-Commit-Position: refs/heads/master@{#65392}

Refs: v8/v8@d406bfd

dmabupt commented Dec 11, 2019

Copy link
Copy Markdown
Contributor Author

Seems this test case failed?

=== release test-process-env-allowed-flags-are-documented ===
Path: parallel/test-process-env-allowed-flags-are-documented
--- stderr ---
assert.js:99
  throw new AssertionError(obj);
  ^
AssertionError [ERR_ASSERTION]: The following options are documented as allowed in NODE_OPTIONS in /home/travis/build/nodejs/node/doc/api/cli.md: --insecure-http-parser but are not in process.allowedNodeEnvironmentFlags

richardlau commented Dec 11, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

Seems this test case failed?

=== release test-process-env-allowed-flags-are-documented ===
Path: parallel/test-process-env-allowed-flags-are-documented
--- stderr ---
assert.js:99
  throw new AssertionError(obj);
  ^
AssertionError [ERR_ASSERTION]: The following options are documented as allowed in NODE_OPTIONS in /home/travis/build/nodejs/node/doc/api/cli.md: --insecure-http-parser but are not in process.allowedNodeEnvironmentFlags

I think that's a race condition between 02a0c74 landing on master and the last update to the commits in this PR. I've restarted the Travis CI.

BridgeAR added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. and removed wip Issues and PRs that are still a work in progress. labels Dec 24, 2019

nodejs-github-bot commented Dec 24, 2019

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Dec 25, 2019

Copy link
Copy Markdown
Collaborator

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

BridgeAR pushed a commit that referenced this pull request Dec 25, 2019
test: skip the unsupported test cases for IBM i
262c66a 
This is a following PR of #30714.

PR-URL: #30819
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
BridgeAR pushed a commit that referenced this pull request Dec 25, 2019
deps: V8: cherry-pick d406bfd64653
bbc032d 
Original commit message:

    [base] Fix the return of ClockNow on IBMi

    The API thread_cputime() is only defined but not yet implemented on IBMi.

    Change-Id: I8ea7ff724e749f537b54e75a00d718500807ca8a
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1957831
    Reviewed-by: Junliang Yan <jyan@ca.ibm.com>
    Reviewed-by: Clemens Backes <clemensb@chromium.org>
    Commit-Queue: Milad Farazmand <miladfar@ca.ibm.com>
    Cr-Commit-Position: refs/heads/master@{#65392}

Refs: v8/v8@d406bfd

PR-URL: #30819
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

BridgeAR commented Dec 25, 2019

Copy link
Copy Markdown
Member

Landed in 262c66a, bbc032d 🎉

BridgeAR closed this Dec 25, 2019
dmabupt deleted the ibmi_skipcase branch January 2, 2020 02:01
BridgeAR pushed a commit that referenced this pull request Jan 3, 2020
test: skip the unsupported test cases for IBM i
e28e873 
This is a following PR of #30714.

PR-URL: #30819
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
BridgeAR pushed a commit that referenced this pull request Jan 3, 2020
deps: V8: cherry-pick d406bfd64653
7fe8399 
Original commit message:

    [base] Fix the return of ClockNow on IBMi

    The API thread_cputime() is only defined but not yet implemented on IBMi.

    Change-Id: I8ea7ff724e749f537b54e75a00d718500807ca8a
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1957831
    Reviewed-by: Junliang Yan <jyan@ca.ibm.com>
    Reviewed-by: Clemens Backes <clemensb@chromium.org>
    Commit-Queue: Milad Farazmand <miladfar@ca.ibm.com>
    Cr-Commit-Position: refs/heads/master@{#65392}

Refs: v8/v8@d406bfd

PR-URL: #30819
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
BridgeAR mentioned this pull request Jan 7, 2020
Merged
targos pushed a commit that referenced this pull request Jan 14, 2020
test: skip the unsupported test cases for IBM i
9c9138f 
This is a following PR of #30714.

PR-URL: #30819
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos pushed a commit that referenced this pull request Jan 14, 2020
deps: V8: cherry-pick d406bfd64653
86d1998 
Original commit message:

    [base] Fix the return of ClockNow on IBMi

    The API thread_cputime() is only defined but not yet implemented on IBMi.

    Change-Id: I8ea7ff724e749f537b54e75a00d718500807ca8a
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1957831
    Reviewed-by: Junliang Yan <jyan@ca.ibm.com>
    Reviewed-by: Clemens Backes <clemensb@chromium.org>
    Commit-Queue: Milad Farazmand <miladfar@ca.ibm.com>
    Cr-Commit-Position: refs/heads/master@{#65392}

Refs: v8/v8@d406bfd

PR-URL: #30819
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos mentioned this pull request Jan 15, 2020
Closed
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
test: skip the unsupported test cases for IBM i
c7dafd8 
This is a following PR of #30714.

PR-URL: #30819
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
deps: V8: cherry-pick d406bfd64653
76eaf24 
Original commit message:

    [base] Fix the return of ClockNow on IBMi

    The API thread_cputime() is only defined but not yet implemented on IBMi.

    Change-Id: I8ea7ff724e749f537b54e75a00d718500807ca8a
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1957831
    Reviewed-by: Junliang Yan <jyan@ca.ibm.com>
    Reviewed-by: Clemens Backes <clemensb@chromium.org>
    Commit-Queue: Milad Farazmand <miladfar@ca.ibm.com>
    Cr-Commit-Position: refs/heads/master@{#65392}

Refs: v8/v8@d406bfd

PR-URL: #30819
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins mentioned this pull request Feb 8, 2020
Merged
MoLow mentioned this pull request Jun 18, 2023
Merged
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

bnoordhuis bnoordhuis left review comments
cjihrig cjihrig left review comments
richardlau richardlau left review comments
jasnell jasnell approved these changes
addaleax addaleax approved these changes
BridgeAR BridgeAR approved these changes

Assignees

No one assigned

Labels

async_hooks Issues and PRs related to the async hooks subsystem. author ready PRs that have at least one approval, no outstanding review comments, and a CI started. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

Back | FazBrowse Home | New Git URL