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
Closed

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

wants to merge 2 commits into from

Conversation

dmabupt
Copy link
Contributor

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

@nodejs-github-bot 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
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 bnoordhuis left a comment

Choose a reason for hiding this comment

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

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
test/sequential/test-inspector-contexts.js Show resolved Hide resolved
@sam-github
Copy link
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 sam-github added the wip Issues and PRs that are still a work in progress. label Dec 6, 2019
@dmabupt
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
Copy link
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)

[$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
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
Copy link
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]).

tools/utils.py Outdated Show resolved Hide resolved
@dmabupt dmabupt marked this pull request as ready for review December 9, 2019 05:28
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
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
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.

@BridgeAR BridgeAR added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed wip Issues and PRs that are still a work in progress. labels Dec 24, 2019
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

BridgeAR pushed 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 pushed 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
Copy link
Member

Landed in 262c66a, bbc032d 🎉

@BridgeAR BridgeAR closed this Dec 25, 2019
@dmabupt dmabupt deleted the ibmi_skipcase branch January 2, 2020 02:01
BridgeAR pushed 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 pushed 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 pushed 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 pushed 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>
BethGriggs pushed a commit that referenced this pull request Feb 6, 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>
BethGriggs pushed a commit that referenced this pull request Feb 6, 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>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_hooks Issues and PRs related to the async hooks subsystem. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants