refactor: set default test options #1704
Conversation
lili2311
force-pushed
the
refactor/set-default-test-options
branch
from
5e4b887 to
c12977a
Compare
| import * as config from '../../../lib/config'; | ||
| import { Options, ShowVulnPaths, TestOptions } from '../../../lib/types'; | ||
|
|
||
| export function setDefaultTestOptions( |
There was a problem hiding this comment.
I believe we should keep adding jest-unit test for these new modules
There was a problem hiding this comment.
how are these structured? Right now I am dumping all new tests into top level test directory because of the jest pattern restrictions 😢 can we update this to allow us to add tests in correct folder structures instead?
- code:
unit/src/lib/index.ts - test:
test/lib/index.spec.ts? Maybe for nowtest/jest/unit/lib/index.spec.tsuntil we fully migrate to Jest?
It will be hard to separate jest tests later from system/acceptance/unit etc
There was a problem hiding this comment.
Test added, please advise if it can move somewhere better than top level of test directory
|
|
||
| export function setDefaultTestOptions(options: Options): Options & TestOptions { | ||
| const svpSupplied = (options['show-vulnerable-paths'] || '') | ||
| .toString() |
There was a problem hiding this comment.
this essentially adds support for --show-vulnerable-paths to be treated as --show-vulnerable-paths=true WDYT?
Otherwise if you run test with snyk test --show-vulnerable-paths you get a bad error that says
toLowerCase is not a function because it is not a string. Or alternative is to add validation for this options and throw outside of this function somewhere probably
There was a problem hiding this comment.
👍 this sounds in line with how arg parsing libraries should treat this
There was a problem hiding this comment.
@gitphill while @aviadhahami is off, could you help 👀 ?
lili2311
force-pushed
the
refactor/set-default-test-options
branch
from
c47c9db to
0545ea6
Compare
lili2311
force-pushed
the
refactor/set-default-test-options
branch
from
0545ea6 to
249a43d
Compare
| @@ -13,8 +13,7 @@ export type DepTree = legacyApi.DepTree; | |||
| export type ShowVulnPaths = 'none' | 'some' | 'all'; | |||
|
|
|||
| export interface TestOptions { | |||
| traverseNodeModules: boolean; | |||
| interactive: boolean; | |||
There was a problem hiding this comment.
this is a protect option for wizard only
|
|
||
| export function setDefaultTestOptions(options: Options): Options & TestOptions { | ||
| const svpSupplied = (options['show-vulnerable-paths'] || '') | ||
| .toString() |
There was a problem hiding this comment.
👍 this sounds in line with how arg parsing libraries should treat this
| it('defaults to show-vulnerable-paths:some & org from config when no options passed in', () => { | ||
| const updated = setDefaultTestOptions({ path: '/' }); | ||
| expect(updated).toEqual({ | ||
| org: undefined, |
There was a problem hiding this comment.
Just curious, in the jest's equals logic, is org: undefined same as not having org key at all?
lili2311
force-pushed
the
refactor/set-default-test-options
branch
from
249a43d to
0f2ee40
Compare
|
Just a nit, noticed a typo in the commit message, which would propagate it to the release notes: features:
- delete vuln paths option once transofrmed + tests
+ delete vuln paths option once transformed |
Add tests for the setDefaultTestOptions()
lili2311
force-pushed
the
refactor/set-default-test-options
branch
from
0f2ee40 to
442e37f
Compare
|
Expected release notes (by @lili2311) features: others (will not be included in Semantic-Release notes):
|
What does this PR do?
refactor settings default test options so it can be re-used in another command that relies on test running first