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: skip the unsupported test cases for IBM i #30819

Closed
wants to merge 2 commits into from

Conversation

@dmabupt
Copy link
Contributor

dmabupt commented Dec 6, 2019

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
@dmabupt dmabupt force-pushed the dmabupt:ibmi_skipcase branch from d3c427e to 9ccee56 Dec 6, 2019
test/parallel/test-c-ares.js Outdated Show resolved Hide resolved
test/parallel/test-child-process-uid-gid.js Outdated Show resolved Hide resolved
test/async-hooks/test-fseventwrap.js Outdated Show resolved Hide resolved
test/parallel/test-c-ares.js Outdated Show resolved Hide resolved
test/parallel/test-child-process-fork-net-server.js Outdated Show resolved Hide resolved
test/parallel/test-cli-node-options.js Outdated Show resolved Hide resolved
test/parallel/test-fs-watch-close-when-destroyed.js Outdated Show resolved Hide resolved
test/parallel/test-http-writable-true-after-close.js Outdated Show resolved Hide resolved
Copy link
Member

bnoordhuis left a comment

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

test/async-hooks/test-fseventwrap.js Outdated Show resolved Hide resolved
test/parallel/test-child-process-fork-net-server.js Outdated Show resolved Hide resolved
test/parallel/test-http-writable-true-after-close.js Outdated Show resolved Hide resolved
test/parallel/test-module-loading-error.js Outdated Show resolved Hide resolved
test/parallel/test-os.js Outdated Show resolved Hide resolved
@sam-github

This comment has been minimized.

Copy link
Member

sam-github commented Dec 6, 2019

@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?

@dmabupt

This comment has been minimized.

Copy link
Contributor Author

dmabupt commented Dec 7, 2019

@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

This comment has been minimized.

Copy link
Member

richardlau commented Dec 7, 2019

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)

[$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

This comment has been minimized.

Copy link
Contributor Author

dmabupt commented Dec 7, 2019

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)

[$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

This comment has been minimized.

Copy link
Member

richardlau commented Dec 7, 2019

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]).

@dmabupt dmabupt force-pushed the dmabupt:ibmi_skipcase branch 2 times, most recently from bc2a86e to 0ae46df Dec 8, 2019
tools/utils.py Outdated Show resolved Hide resolved
@dmabupt dmabupt force-pushed the dmabupt:ibmi_skipcase branch from 5ef557b to f18d467 Dec 8, 2019
This is a following PR of #30714.
@dmabupt dmabupt marked this pull request as ready for review Dec 9, 2019
@dmabupt dmabupt force-pushed the dmabupt:ibmi_skipcase branch from f18d467 to ae4905a Dec 9, 2019
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

This comment has been minimized.

Copy link
Contributor Author

dmabupt commented Dec 11, 2019

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

This comment has been minimized.

Copy link
Member

richardlau commented Dec 11, 2019

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.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

BridgeAR added a commit that referenced this pull request Dec 25, 2019
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 added a commit that referenced this pull request Dec 25, 2019
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

This comment has been minimized.

Copy link
Member

BridgeAR commented Dec 25, 2019

Landed in 262c66a, bbc032d 🎉

@BridgeAR BridgeAR closed this Dec 25, 2019
@dmabupt dmabupt deleted the dmabupt:ibmi_skipcase branch Jan 2, 2020
BridgeAR added a commit that referenced this pull request Jan 3, 2020
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 added a commit that referenced this pull request Jan 3, 2020
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 BridgeAR mentioned this pull request Jan 7, 2020
targos added a commit that referenced this pull request Jan 14, 2020
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 added a commit that referenced this pull request Jan 14, 2020
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.