-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat: Added Form instances and selectors, added all tests #67
base: develop
Are you sure you want to change the base?
Conversation
321407d
to
5cb634f
Compare
active: false, | ||
}, | ||
], | ||
path: 'https://query.advancecomunicacao.com.br/v2/workspaces/workspace1/singletons', |
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.
Please update this 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.
There are a few things that I'm concerned about breaking changes. I'm a bit out of context why the refactoring of a few functions/classes. I'm up for a call so we can talk through the changes so I can understand the reasoning behind them.
But honestly, that's a fine work there! Really excited to get these tests in! 🕺
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.
Do we really need all this data in this fixture? It's usually better to have as little as possible in a fixture so it's easier to maintain and update when needed
import { collectionInfo, collectionItems } from '../../../mocks/fixtures' | ||
|
||
describe('CollectionInstance methods', () => { | ||
it("should return the proper collection object (using collection('collectionName'))", async () => { |
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("should return the proper collection object (using collection('collectionName'))", async () => { | |
it("should return the proper collection object)", async () => { |
I think it's unnecessary to add the function in the description as this file only has tests related to that function. Also the function name is already in the import
expect(collection).toEqual(collectionInfo) | ||
}) | ||
|
||
it("should return the proper collection items (using collection('collectionName'))", async () => { |
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.
♻️ same here
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'm not familiar with the SDK's types but will these change break anything for the consumer of the library? If so, could you explain why these changes are needed?
src/instances/Form/index.spec.ts
Outdated
import { formInfo, formSchema } from '../../../mocks/fixtures' | ||
|
||
describe('FormInstance methods', () => { | ||
it("should return the proper form object (using form('formName'))", async () => { |
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.
♻️ Same as the previous comment about naming the test
} | ||
|
||
return Reflect.get(target, prop) | ||
}, | ||
}) as DynamicCollectionSelector | ||
}) as unknown as DynamicCollectionSelector |
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.
Can't say I'm a fan of this type. Any other option for this?
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.
Same as before, do we need all this data?
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.
♻️ Same here. Can we get a smaller fixture with less data?
fix #30