[core] Add snapshot update feature to promise logic#5132
[core] Add snapshot update feature to promise logic#5132davidkpiano wants to merge 3 commits intomainfrom
Conversation
|
|
|
||
| expect(stuff).toEqual( | ||
| expect.arrayContaining([ | ||
| expect.objectContaining({}), |
There was a problem hiding this comment.
it feels like a better "assertion" could be put here, this one looks suspicious - maybe we could use { status: 'active' } or smth like that?
There was a problem hiding this comment.
then it would make sense to include the status in all of those assertions~
| value: 'loading', | ||
| context: { progress: 0.6 } | ||
| }), | ||
| expect.objectContaining({ value: 'finished', context: undefined }), |
There was a problem hiding this comment.
I feel like this test case is too big. "can emit updates" is too broad and doesn't give me much insight into what is actually expected because it reads as "it works".
For instance, I noticed that an update with just one of those fields would wipe out the other one. I was wondering how deliberate that was and if we have a test for it. It turns out we do - but it's just so buried in this item here. It's hard to tell if this was intentional or accidental. I think it was intentional and that's fine - but my point stands: a test with so many details in it isn't particularly informative/explicit about its intentions
| export interface PromiseState { | ||
| value?: StateValue; | ||
| context?: MachineContext; | ||
| } |
There was a problem hiding this comment.
it would be nice to add some type-level tests for usage with discriminated unions, including tests that would exercise .getSnapshot()
Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>
2 participants
No description provided.