test: Enable specifying flaky tests on fips #16329

komawar · 2017-10-20T01:10:04Z

Adds a way to mark a specified test as 'flaky' on fips compliant
systems.

Earlier, the tools/test.py script supported only 'mode',
'system' and 'arch' for test environment specification. This limits the
ability to specify the behavior of tests and setting pre-determined
behavior of the same on other types of systems. As an example, the
feature request below indicates the need to specify certain tests as
'flaky' on fips compliant systems. It hints at future possibility of a
shared library, which in turn may need a specifier for running tests.

This commit introduces a new item in the env dict, called type
which defaults to simple type. It also adds an optional command
line argument --type, which inputs strings. Current functionality
extends to setting simple or fips for this type variable.
However, extending it to further uses is rather simple by adding "if"
conditions at appropriate places in the tools/test.py script.

Fixes: #14746

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

build
doc
test

refack · 2017-10-20T02:06:53Z

@komawar thanks!

Could you add a "negative control"? a test that fails if hasFipsCrypto, in test/parallel something like:

const common = require('../common');
if (common.hasFipsCrypto) process.exit(1);

Then add an exception in parallel.status. That would serve as documentation as well.

komawar · 2017-10-20T02:13:06Z

@refack Thanks for your review! Surely, let me look into it / wrap my head around this.

komawar · 2017-10-20T02:14:52Z

Just wanted to leave a comment (ease the load on reviewers) on a sample test run which I conducted, here's the commit that details the run and marked tests as flaky (for testing purposes):
komawar@4a957ca

refack · 2017-10-20T02:15:31Z

Sure. Take a look at the current status file:

node/test/parallel/parallel.status

Lines 1 to 22 in c3ae57f

    
           prefix parallel 
        
           # To mark a test as flaky, list the test name in the appropriate section 
        
           # below, without ".js", followed by ": PASS,FLAKY". Example: 
        
           # sample-test                        : PASS,FLAKY 
        
           [true] # This section applies to all platforms 
        
           [$system==win32] 
        
           [$system==linux] 
        
           [$system==macos] 
        
           [$arch==arm || $arch==arm64] 
        
           test-npm-install:      PASS,FLAKY 
        
           [$system==solaris] # Also applies to SmartOS 
        
           [$system==freebsd] 
        
           [$system==aix]

komawar · 2017-10-20T02:17:23Z

@refack Oh I get it now! (I misread what you were saying there). Surely, I was hoping to get a pointer on how to best test this and here it is! perfect, I will add this.

refack · 2017-10-20T02:21:25Z

Maybe a most verbose way to "fail" the test is to use assert:

if (common.hasFipsCrypto)
  assert.fail(`expected to fail with FIPS enabled`)

gibfahn

Couple of thoughts

gibfahn · 2017-10-24T11:35:31Z

test/parallel/test-negative-on-fips.js

+// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
+// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
+// USE OR OTHER DEALINGS IN THE SOFTWARE.


Copyright is not needed in new files AIUI (cc/ @jasnell for confirmation).

This file has been removed as per @Trott 's suggestion.

gibfahn · 2017-10-24T11:40:14Z

Makefile

@@ -505,6 +505,9 @@ test-v8 test-v8-intl test-v8-benchmarks test-v8-all:
 		"$ git clone https://github.com/nodejs/node.git"
 endif

+test-on-fips:
+	$(PYTHON) tools/test.py --type=fips


I don't think a separate command is needed here, we have way too many commands as it is. Also we'd really need to provide a fips option for every make target that includes tools/test.py. I think what would be helpful is a way to pass arguments to tools/test.py calls using an environment variable, simillar to CONFIG_FLAGS. That should be done separately, so I'll raise a separate issue.

ok. do you want me to wait until that's been discussed or should we go with something short term here?

Thanks for that insight. The latest PR excludes these changes.

gibfahn · 2017-10-24T11:41:48Z

tools/test.py

@@ -1427,6 +1427,9 @@ def BuildOptions():
  result.add_option('--abort-on-timeout',
      help='Send SIGABRT instead of SIGTERM to kill processes that time out',
      default=False, action="store_true", dest="abort_on_timeout")
+  result.add_option("--type",
+      help="Specifies the environment type the tests will run on, for example 'fips'",


So currently this only supports fips right? I think we should give an exhaustive list of the possible options, so maybe something like:

Specifies the type of build. Options: fips"

That way we can easily expand the list when we add more options (like shared).

Otherwise the for example is misleading.

gibfahn · 2017-10-24T11:48:11Z

test/parallel/test-negative-on-fips.js

+// This test will fail on fips. The corresponding line in
+// parallel.status file makes sure this is marked as flaky and passes.
+'use strict';
+const common = require('../common');


Can you order these as per the writing tests guide? Basically use strict, require common, comment.

sure. thanks for pointing that out.

komawar · 2017-10-25T18:14:38Z

Thanks for your reviews. I will wait until more comments, confirmations and direction has been discussed to update this.

Trott · 2017-10-25T20:59:02Z

BUILDING.md

@@ -135,6 +135,15 @@ $ make test
 At this point you are ready to make code changes and re-run the tests!
 Optionally, continue below.

+If you wish to run tests on specific systems, say ``fips``:


Nit: FIPS is not a system. (Is it more correct to call it a "configuration"?)

This file has been removed as per your suggestion.

Trott · 2017-10-25T20:59:30Z

BUILDING.md

+```
+
+This particular command will mark the tests specified in the \*.status files as
+flaky.


This is confusing. Running make test-on-fips will not mark tests as flaky.

Trott · 2017-10-25T21:01:56Z

test/parallel/parallel.status

+# Sample test (test-negative-on-fips) has been added to check the
+# skip-on-fips feature (issue 14746)
+[$type==fips]
+test-negative-on-fips: PASS,FLAKY


I don't think this should be part of the PR nor the associated test. We don't have permanent entries in the *.status files. If something is in there, it's because the test is having a problem that needs to be addressed.

I don't believe we generally test the test.py code directly like this. That may not be a great practice, but trying to introduce it in this PR is probably not the way to go.

This file has been removed as per your suggestion.

Trott · 2017-10-25T21:04:09Z

This is great and thanks for doing it! I think we only need the changes in test.py here. The changes to Makefile, parallel.status, the new test, and the documentation seem extraneous, which seems weird to say. "Hey, nice PR, but can you remove the tests and the documentation?" Yeah, that suggests a problem with our process. :-D You were probably just following the checklist so apologies for having our documentation make you do work that maybe wasn't necessary. :-(

All that said, I would be excited to have a working skip-FIPS feature in test.py!!!!!! Let's do this thing!

komawar · 2017-11-07T18:22:52Z

@Trott @refack @gibfahn thanks for your reviews. I was hoping that we sort of have a convergence of opinion for this conversation (given the views of three of your differed on the reviews) but in the interest of time, I have avoided the extra commits and the latest PR has just the test.py changes as per @Trott 's suggestion. Hopefully, that is okay with everyone referenced in the review process (and any following reviews)!

I will create a different PR indicating that changes to test.py do not need (for most cases) the related changes as in this earlier PR.

cheers

Trott · 2017-11-08T16:32:38Z

tools/test.py

@@ -1427,6 +1427,9 @@ def BuildOptions():
  result.add_option('--abort-on-timeout',
      help='Send SIGABRT instead of SIGTERM to kill processes that time out',
      default=False, action="store_true", dest="abort_on_timeout")
+  result.add_option("--type",
+      help="Specifies the type of build. Options: fips"


Nit (non-blocking): Can we format this more like existing options?

For example, using this as a model:

"The style of progress indicator (verbose, dots, color, mono, tap)"

...this would be:

"Type of build (default, fips)"

...or don't list the options at all if they're not enumerated by this file (which I don't see where they are).

The options are default or fips AFAIK.

Yeah, the options are default or fips.

You don't see the enumeration as there are no more choices and the existing code parses the type = fips to give you PASS, FAIL, SKIP functionality. It took me some time to figure this out, code is crazy old, pretty complicated to read and I do intend to follow up with a blog entry on this soon(ish), for knowledge sharing.

... also this will have to change a bit for shared case when we might see some enumertaion

@Trott (formatting has been done in latest)

Trott · 2017-11-08T16:37:19Z

There isn't any pre-existing functionality to already skip a test if an entry for type of fips if I include something like this in a .status file, is there?

[$type==fips]
test-foo-bar: SKIP,FLAKY

komawar · 2017-11-08T19:31:31Z

@Trott

There isn't any pre-existing functionality to already skip a test if an entry for type of fips if I include something like this in a .status file, is there?

Yes, it should work out-of-the-box for:-

[$type==fips]
test-foo-bar: SKIP,FLAKY

Please see an example test I've created here komawar@4a957ca , (had referenced this before but is hard to find so that's okay).

This is one of the reasons I had added the negative sample test here komawar@5bf0d6a as suggested by @refack , itself acts as a bit documentation. But I do agree that we prolly shouldn't change the structure/type of tests in the repo like you mentioned earlier.

Maybe we can have a "mock" test for such things but I don't see where it exists for tools/test.py -- ideas appreciated. May be something for future?

gibfahn · 2017-11-08T22:29:42Z

So actually I think what would be really useful is if tools/test.py could auto-detect that you're running a fips build, and set the type accordingly.

So instead of having to run tools/test.py --type=fips, you'd just run tools/test.py, and it would work out that node was a fips-enabled one.

You can check that by doing node -p process.versions.openssl and checking that it contains the string fips.

@komawar if you'd be up for implementing that that would be amazing. Let me know if you get stuck anywhere, I did something similar in this file recently, so could help out with it if needed.

komawar · 2017-11-09T14:18:01Z

So instead of having to run tools/test.py --type=fips, you'd just run tools/test.py, and it would work out that node was a fips-enabled one.

You can check that by doing node -p process.versions.openssl and checking that it contains the string fips.

@gibfahn no issues, this looks like a good idea. gonna go at it..

However, I still think we need the --type, may be for future purposes or if you wanted to enforce default (or any other) behavior on specific systems. Thoughts?

gibfahn · 2017-11-09T14:40:34Z

However, I still think we need the --type, may be for future purposes or if you wanted to enforce default (or any other) behavior on specific systems. Thoughts?

I think you're right, having the --type override the guessed version makes sense. And yes, we'll definitely want more types going forward.

komawar · 2017-11-09T22:58:25Z

So actually I think what would be really useful is if tools/test.py could auto-detect that you're running a fips build, and set the type accordingly.

This has been done.

Adds a way to mark a specified test as 'flaky' on fips compliant systems. Earlier, the ``tools/test.py`` script supported only 'mode', 'system' and 'arch' for test environment specification. This limits the ability to specify the behavior of tests and setting pre-determined behavior of the same on other types of systems. As an example, the feature request below indicates the need to specify certain tests as 'flaky' on fips compliant systems. It hints at future possibility of a shared library, which in turn may need a specifier for running tests. This commit introduces a new item in the ``env`` dict, called ``type`` which defaults to ``simple`` type. It also adds an optional command line argument ``--type``, which inputs strings. Current functionality extends to setting ``simple`` or ``fips`` for this ``type`` variable. However, extending it to further uses is rather simple by adding "if" conditions at appropriate places in the ``tools/test.py`` script. Fixes: issue 14746

gibfahn · 2017-11-11T00:09:05Z

@Trott what do you think?

Trott · 2017-11-13T00:20:31Z

@gibfahn No objections!

BridgeAR · 2017-11-23T10:37:41Z

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

addaleax · 2017-11-28T02:00:37Z

Landed in f31cf56

Adds a way to mark a specified test as 'flaky' on fips compliant systems. Earlier, the ``tools/test.py`` script supported only 'mode', 'system' and 'arch' for test environment specification. This limits the ability to specify the behavior of tests and setting pre-determined behavior of the same on other types of systems. As an example, the feature request below indicates the need to specify certain tests as 'flaky' on fips compliant systems. It hints at future possibility of a shared library, which in turn may need a specifier for running tests. This commit introduces a new item in the ``env`` dict, called ``type`` which defaults to ``simple`` type. It also adds an optional command line argument ``--type``, which inputs strings. Current functionality extends to setting ``simple`` or ``fips`` for this ``type`` variable. However, extending it to further uses is rather simple by adding "if" conditions at appropriate places in the ``tools/test.py`` script. PR-URL: #16329 Fixes: #14746 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

refack · 2017-11-29T00:38:41Z

FYI: This change uses subprocess.check_output which is a python 2.7 only feature
It is failing on our Centos6 CI workers - https://ci.nodejs.org/job/node-test-commit-linux/14398/nodes=centos6-64/console

For the time being I've updated the workers to use python2.7 and opened #17381 to improve this change.

/CC @nodejs/build

gibfahn · 2017-11-29T00:44:24Z

Thanks for fixing @refack , wonder why it didn't fail pre-land CI.

refack · 2017-11-29T00:46:39Z

wonder why it didn't fail pre-land CI.

It did - node-test-commit-linux/14398 was part or the above mentioned node-test-pull-request/11661

refack · 2017-11-29T00:58:37Z

So apparently it's not simple to upgrade CentOS6 to python2.7, so I'd rather fast-track #17381 then fiddle around with the system config.

Adds a way to mark a specified test as 'flaky' on fips compliant systems. Earlier, the ``tools/test.py`` script supported only 'mode', 'system' and 'arch' for test environment specification. This limits the ability to specify the behavior of tests and setting pre-determined behavior of the same on other types of systems. As an example, the feature request below indicates the need to specify certain tests as 'flaky' on fips compliant systems. It hints at future possibility of a shared library, which in turn may need a specifier for running tests. This commit introduces a new item in the ``env`` dict, called ``type`` which defaults to ``simple`` type. It also adds an optional command line argument ``--type``, which inputs strings. Current functionality extends to setting ``simple`` or ``fips`` for this ``type`` variable. However, extending it to further uses is rather simple by adding "if" conditions at appropriate places in the ``tools/test.py`` script. PR-URL: #16329 Fixes: #14746 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

gibfahn · 2017-12-19T22:32:48Z

This would be good to have on v6.x-staging? Could someone backport this with #17381? Guide is here.

Adds a way to mark a specified test as 'flaky' on fips compliant systems. Earlier, the ``tools/test.py`` script supported only 'mode', 'system' and 'arch' for test environment specification. This limits the ability to specify the behavior of tests and setting pre-determined behavior of the same on other types of systems. As an example, the feature request below indicates the need to specify certain tests as 'flaky' on fips compliant systems. It hints at future possibility of a shared library, which in turn may need a specifier for running tests. This commit introduces a new item in the ``env`` dict, called ``type`` which defaults to ``simple`` type. It also adds an optional command line argument ``--type``, which inputs strings. Current functionality extends to setting ``simple`` or ``fips`` for this ``type`` variable. However, extending it to further uses is rather simple by adding "if" conditions at appropriate places in the ``tools/test.py`` script. PR-URL: #16329 Fixes: #14746 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

nodejs-github-bot added test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. labels Oct 20, 2017

refack self-assigned this Oct 20, 2017

komawar force-pushed the enable-flaky-on-fips branch from b8ada86 to 5bf0d6a Compare October 20, 2017 03:32

gibfahn reviewed Oct 25, 2017

View reviewed changes

Trott reviewed Oct 25, 2017

View reviewed changes

Trott added the python PRs and issues that require attention from people who are familiar with Python. label Oct 25, 2017

komawar force-pushed the enable-flaky-on-fips branch from 5bf0d6a to d867ff3 Compare November 7, 2017 18:14

gibfahn approved these changes Nov 7, 2017

View reviewed changes

jasnell approved these changes Nov 8, 2017

View reviewed changes

Trott reviewed Nov 8, 2017

View reviewed changes

komawar force-pushed the enable-flaky-on-fips branch from d867ff3 to fd1c661 Compare November 8, 2017 19:28

komawar force-pushed the enable-flaky-on-fips branch from fd1c661 to 74654f0 Compare November 9, 2017 22:56

komawar force-pushed the enable-flaky-on-fips branch from 2950653 to b0c0dee Compare November 10, 2017 05:55

BridgeAR approved these changes Nov 23, 2017

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 23, 2017

addaleax closed this Nov 28, 2017

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 28, 2017

refack removed their assignment Nov 29, 2017

MylesBorins mentioned this pull request Dec 12, 2017

v9.3.0 proposal #17631

Merged

gibfahn added backport-requested-v6.x labels Dec 19, 2017

This was referenced Dec 19, 2017

tools,test: use Execute instead of check_output #17381

Merged

v8.9.4 proposal #17772

Closed

gibfahn mentioned this pull request Dec 20, 2017

v8.9.4 proposal #17774

Merged

test: Enable specifying flaky tests on fips #16329

test: Enable specifying flaky tests on fips #16329

Conversation

komawar commented Oct 20, 2017 • edited Loading

Checklist

Affected core subsystem(s)

refack commented Oct 20, 2017

komawar commented Oct 20, 2017

komawar commented Oct 20, 2017

refack commented Oct 20, 2017

komawar commented Oct 20, 2017

refack commented Oct 20, 2017

gibfahn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

komawar commented Oct 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

komawar Nov 7, 2017 • edited Loading

Choose a reason for hiding this comment

Trott commented Oct 25, 2017

komawar commented Nov 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott commented Nov 8, 2017 • edited Loading

komawar commented Nov 8, 2017

gibfahn commented Nov 8, 2017

komawar commented Nov 9, 2017 • edited Loading

gibfahn commented Nov 9, 2017

komawar commented Nov 9, 2017

gibfahn commented Nov 11, 2017

Trott commented Nov 13, 2017

BridgeAR commented Nov 23, 2017

addaleax commented Nov 28, 2017

refack commented Nov 29, 2017

gibfahn commented Nov 29, 2017

refack commented Nov 29, 2017

refack commented Nov 29, 2017

gibfahn commented Dec 19, 2017

komawar commented Oct 20, 2017 •

edited

Loading

komawar Nov 7, 2017 •

edited

Loading

Trott commented Nov 8, 2017 •

edited

Loading

komawar commented Nov 9, 2017 •

edited

Loading