-
Notifications
You must be signed in to change notification settings - Fork 80
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
fix: Fix an issue where invalid VerifierOptions keys with Array values would cause a TypeError #432
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -47,6 +47,18 @@ describe('Verifier argument validator', () => { | |
}); | ||
}); | ||
}); | ||
|
||
context('when given an unknown array argument', () => { | ||
it('should return a Verifier object', () => { | ||
expectSuccessWith({ | ||
madeupArg: [''], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. would js treat and would that make a diff for this logic There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nope 👍
You can see the same |
||
providerBaseUrl: 'http://localhost', | ||
pactBrokerUrl: 'http://foo.com', | ||
pactUrls: ['http://idontexist'], | ||
provider: 'someprovider', | ||
} as VerifierOptions); | ||
}); | ||
}); | ||
}); | ||
|
||
context('when not given --pact-urls or --provider-base-url', () => { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the user be aware that an invalid option was provided, and bail because of it?
otherwise they might feel that the argument is correctly being set, but it isn't
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about adding a warning - but the existing behaviour (for unknown options that don't have an array value) was to silently ignore them.
I'm not sure if there are some unvalidated options - if we want to warn, we'll have to double check the list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmm, whilst that wouldn't be my preference, it is the way it is currently so I wouldn't want to break userspace too much.
looking at the users provided file, the only invalid option I think was
tags
which is nowproviderVersionTags
andlog
option is no longer in v10 of pact-js post moving to the rust core.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that is probably one for another PR tho!