aix: improve readability of os version #57
Conversation
|
I'll add a test, just need to think of a sensible thing to check (maybe for AIX that it conforms the regex |
|
Do we actually want to change so we pretty print AIX version info? At the moment the output format is similar to the format from uname -a: I agree AIX's output format is odd but is it better to be consistent with that as it is what the user will see when they check the version string on their box? |
https://www.ibm.com/support/knowledgecenter/en/ssw_aix_72/com.ibm.aix.cmds4/oslevel.htm |
uname on AIX reports, e.g. 6.1 as version == 6, release == 1. The code was previously printing this as: OS version: AIX 1 6 Fix so that on AIX this is now: OS version: AIX 6.1
richardlau
force-pushed
the
aix_osversion
branch
from
a908b21
to
d80c590
Compare
|
Rebased onto master to pick up changes from #48, which changed |
test/test-os-version.js
Outdated
| } else if (common.isAIX() && !os.release().includes('.')) { | ||
| // For Node.js prior to os.release() fix for AIX: https://github.com/nodejs/node/pull/10245 | ||
| tap.match(version_str, new RegExp('OS version: ' + os_name + ' \\d+.' + os.release()), 'Checking OS version'); | ||
| } else if (!common.isWindows()) { |
There was a problem hiding this comment.
Could this just be else since you already check 'if (common.isWindows()) {' ?
test/test-os-version.js
Outdated
| const version_str = report_str.match(/OS version: .*(?:\r*\n)/); | ||
| if (common.isWindows()) { | ||
| tap.match(version_str, new RegExp('OS version: ' + os_name), 'Checking OS version'); | ||
| } else if (common.isAIX() && !os.release().includes('.')) { |
There was a problem hiding this comment.
I don't think .includes() is there in Node v4.
But obviously we can check by running CI with Node v4.
There was a problem hiding this comment.
In a node v4.0.0 repl that seems to work:
$ node-v4.0.0-darwin-x64/bin/node
> os.release().includes('.')
true
It should be fine on v4.
There was a problem hiding this comment.
Sorry, you're right. I think it's Array.includes() that isn't there in v4, string.includes() is.
|
Node v4 CI: https://ci.nodejs.org/view/post-mortem/job/nodereport-continuous-integration/53/ |
uname on AIX reports, e.g. 6.1 as version == 6, release == 1. The code was previously printing this as: OS version: AIX 1 6 Fix so that on AIX this is now: OS version: AIX 6.1 PR-URL: #57 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gibson Fahnestock <gib@uk.ibm.com> Reviewed-By: Howard Hellyer <hhellyer@uk.ibm.com>
|
Landed as 76fa628 |
|
@richardlau I realised after landing this that your testcase test/test-os-version.js is a bit of a departure from our previous test strategy. We were adding the report validation checks into common.js, so that the report content gets checked for all scenarios (i.e. whenever a report is generated). Your new test runs an individual report to check the os version line, so does not give as much coverage? |
uname on AIX reports, e.g. 6.1 as version == 6, release == 1.
The code was previously printing this as:
Fix so that on AIX this is now:
nodejs/node#10245 fixed a similar area in Node.js.