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.

Sign up for GitHub

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

Jump to bottom

Feature Request: mark tests flaky on FIPS #14746

Closed
gibfahn opened this issue Aug 10, 2017 · 4 comments
Closed

Feature Request: mark tests flaky on FIPS #14746

gibfahn opened this issue Aug 10, 2017 · 4 comments
Labels
crypto Issues and PRs related to the crypto subsystem. feature request Issues that request new features to be added to Node.js. python PRs and issues that require attention from people who are familiar with Python. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. wip Issues and PRs that are still a work in progress.

Comments

@gibfahn
Copy link
Member

gibfahn commented Aug 10, 2017

See 8fae1e6#commitcomment-23583689

Currently tools/test.py only compares against env, which is defined as

        env = {
          'mode': mode,
          'system': utils.GuessOS(),
          'arch': vmArch,
        }

Where mode is release/debug, system is OS, and arch is ARCH.

We'd need to also have an option for FIPS, I guess that should be in env. Maybe a type? Type could be default, fips, or sharedlib (if we end up building a shared library as well we might want a sharedlib type).

cc/ @Trott @bajtos
@gibfahn gibfahn added feature request Issues that request new features to be added to Node.js. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. labels Aug 10, 2017
@mscdex mscdex added the crypto Issues and PRs related to the crypto subsystem. label Aug 10, 2017
@Trott Trott added the python PRs and issues that require attention from people who are familiar with Python. label Aug 10, 2017
@bnoordhuis
Copy link
Member

If the buildbots were to set a NODE_BUILDBOT_TYPE environment variable, you could do this:

env = {
  # ...
  'type': os.getenv('NODE_BUILDBOT_TYPE', 'default'),
}

Would that work?

@gibfahn
Copy link
Member Author

gibfahn commented Sep 22, 2017

Well for the fips check you could just do node -p process.versions.openssl.includes('fips'). That would work in local builds as well.

I was thinking more about how to structure the env variable in python.

@komawar
Copy link
Contributor

komawar commented Oct 10, 2017

Hello @gibfahn and others,

I had a small conversation with @Trott offline and I think I understand this issue. I would like to help out with this one if you all are okay with it.

@BridgeAR
Copy link
Member

@komawar please go ahead and open a PR. That is always best.

@refack refack added the wip Issues and PRs that are still a work in progress. label Oct 10, 2017
@refack refack self-assigned this Oct 10, 2017
@komawar komawar mentioned this issue Oct 20, 2017
Closed
4 tasks
komawar added a commit to komawar/node that referenced this issue Oct 20, 2017
@komawar
build: adds a rule in Makefile to test on fips
bf56b05 
The prior commit enables running tests on fips compliant systems by
specifying tests to be marked flaky if needed so. This commit extends
the ability by adding a rule to the Makefile so that tests can be run on
fips compliant systems using the make command.

Fixes: nodejs#14746
komawar added a commit to komawar/node that referenced this issue Oct 20, 2017
@komawar
doc: add doc to specify flaky tests on fips
2fb7825 
This commit adds documentation lines to BUILDING.md file indicating the
changes that have been made to enable marking flaky tests on fips
compliant system. It is an extension to the prior commit that enables
this behavior.

Fixes: nodejs#14746
komawar added a commit to komawar/node that referenced this issue Oct 20, 2017
@komawar
test: Adds a test to check failure on FIPS
5bf0d6a 
A test has been added to the parallel directory which merely exits with
the specified error message on fips. The parallel.status file also has
been modified to mark this test as flaky on fips so, the default test
suite will work without any modifications. This test also serves as
documentation of the added feature metioned below. Inline documentation
has been added to both the files.

Fixes: nodejs#14746
MylesBorins pushed a commit that referenced this issue Dec 12, 2017
@komawar @MylesBorins
test: Enable specifying flaky tests on fips
385653d 
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 <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this issue Dec 12, 2017
@komawar @MylesBorins
test: Enable specifying flaky tests on fips
b416301 
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 <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this issue Dec 12, 2017
@komawar @MylesBorins
test: Enable specifying flaky tests on fips
afdfc4d 
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 <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
gibfahn pushed a commit that referenced this issue Dec 19, 2017
@komawar @gibfahn
test: Enable specifying flaky tests on fips
021d7df 
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 <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
gibfahn pushed a commit that referenced this issue Dec 20, 2017
@komawar @gibfahn
test: Enable specifying flaky tests on fips
86d00b2 
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 <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@refack refack removed their assignment Oct 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
crypto Issues and PRs related to the crypto subsystem. feature request Issues that request new features to be added to Node.js. python PRs and issues that require attention from people who are familiar with Python. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. wip Issues and PRs that are still a work in progress.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

test: Enable specifying flaky tests on fips komawar/node
7 participants
@mscdex @refack @bnoordhuis @Trott @komawar @BridgeAR @gibfahn