-
Notifications
You must be signed in to change notification settings - Fork 3
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 pact-matchers for nested objects #60
Conversation
}, | ||
"$.body.objectInline.nestedArray": Object { | ||
"match": "regex", | ||
"regex": "^[0-9a-fA-F]+$", |
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.
This is incorrect matching rule.
objectInline.nesgtedArray
is not string field, but array, so regex is incorrect
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 think you have created a too complex test case for the test, which is too large and finally, there are difficulties to analyze a lot of snapshot lines.
src/core/type-representation.ts
Outdated
if (typeMatcher && exampleRepresentation) { | ||
|
||
// recursively set type representation for nested object properties | ||
if (propertyType.isObject() && !propertyType.isArray()) { |
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.
It is not solution to this problem.
The problem is in line #90 https://github.com/HLTech/pact-gen-ts/pull/60/files#diff-c6a2f1cd0cfa639da00a7d6c3dfb23df5d133f76e94823ec2619cd05535aa917R90 of this file. We get all descendants of object property (so, if AST contains some JSDocTag node for nested field of some parent field, this is persisted only for parent object field). So, the solution is getting only children nodes of JSDocComments kind of current field.
Working solution:
const jsDocTagsOfProperty = property.getValueDeclaration()?.getChildrenOfKind(ts.SyntaxKind.JSDocComment).reduce((jsDocTags, jsDocCommentElement) => {
return [...jsDocTags, ...(jsDocCommentElement.getChildrenOfKind(ts.SyntaxKind.JSDocTag))];
}, []);
use it, and remove your code :)
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.
Fix things from my comment.
d8e8a93
to
7e7eef6
Compare
tests/int/pactGenerating.test.ts
Outdated
@@ -25,6 +24,16 @@ describe('createPacts', () => { | |||
}); | |||
}); | |||
|
|||
test('pact-matcher', () => { |
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.
Is it purposeful or some test residues?
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 did it as a first step in separating those tests - having them all in one each
makes it a bit more cumbersome to run quickly and get instant feedback. I would even suggest moving all those snapshots into separate files somewhere in the future, we could see in this PR that following and checking everything when it is in one large piece of text is not an easy task.
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.
Actually after consideration - I reverted this change in this PR - lets do it all at once in other refactor task - but the idea stays the same.
package.json
Outdated
@@ -1,6 +1,6 @@ | |||
{ | |||
"name": "pact-gen-ts", | |||
"version": "0.9.0", | |||
"version": "0.9.1", |
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.
You have to update version to 0.9.2.
There has been already 0.9.1 version
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.
Semantic-versioning library is super convenient :) - I guess we could think about implementing it in the near future.
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.
Yeah.
first we need to release version 1.0 :P
Looks nice |
7e7eef6
to
37a9fbe
Compare
Fixes #58