feat: Added Form instances and selectors, added all tests #67
Conversation
RubensDiniz
force-pushed
the
feature/forms
branch
from
321407d
to
5cb634f
Compare
| active: false, | ||
| }, | ||
| ], | ||
| path: 'https://query.advancecomunicacao.com.br/v2/workspaces/workspace1/singletons', |
There was a problem hiding this comment.
Please update this URL.
There was a problem hiding this comment.
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.
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.
| 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.
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.
♻️ 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.
Can't say I'm a fan of this type. Any other option for this?
There was a problem hiding this comment.
Same as before, do we need all this data?
There was a problem hiding this comment.
♻️ Same here. Can we get a smaller fixture with less data?
fix #30