Fix unit tests failing on windows #9
Conversation
stephan-noel
force-pushed
the
fix/unit-tests-failing-windows
branch
2 times, most recently
from
2419ee6 to
c63de5d
Compare
stephan-noel
changed the title
| }); | ||
|
|
||
| function extractHashes( | ||
| result: ExecuteRunner2Result, |
There was a problem hiding this comment.
probably you could use ReturnType<typeof executeRunner2>
There was a problem hiding this comment.
I tried but it didn't work as expected since executeRunner2 returns a Promise, and here I am using the wrapped value of the Promise. I found some workaround using conditional type inference and it seems to work though it's a bit obscure (Unpromise<T>). Do you prefer this approach?
There was a problem hiding this comment.
true, maybe then you can use unpromise to define the ExecuteRunner2Result type so it doesn't need to be kept in sync manually with what function returns, what do you think?
There was a problem hiding this comment.
Hmm, I think I'm not understanding.
I am doing UnPromise<ReturnType<typeof executeRunner2>> now and it works and I deleted the ExecuteRunner2Result so that type inference is used instead and it's working. This way, there is nothing to keep in sync.
Do you want me to put the explicit type back in again?
There was a problem hiding this comment.
Sorry, I meant that in order to not define ad-hoc type helpers which are universal(like unpromise) in that particular test file, you could put them to where executeRunner2 is defined and use to create ExecuteRunner2Result type instead of defining it's detailed structure. But I think we can leave it as it's now as postcss test is the only place using ExecuteRunner2Result. But if you're foreseeing usage of this type in other tests, maybe it makes sense to put ExecuteRunner2Result next to executeRunner2 function and define it through the inference
There was a problem hiding this comment.
Ahh yes, what you're saying makes perfect sense. Also, by doing it that way, external TypeScript users of executeRunner2 can also now more easily define utility functions on the result as they see fit. Changed it as suggested. What do you think?
Also, I squashed and did a bit more manual testing of runner-ts to triple check and everything seems ok. I think we're good to merge now.
stephan-noel
force-pushed
the
fix/unit-tests-failing-windows
branch
from
36a0f2b to
9c993dc
Compare
Description
Fixes #7 This is a partial fix, it won't support npm module syntax in garment.json, only as the extends option in your tsconfig files.
Fixes #3
No relevant documentation changes to be made.
How Has This Been Tested?
Most changes were in unit tests. Source changes (only runner-ts) were also verified manually by running the new code in a standalone project using garment.
Checklist:
Disclaimer
By sending us your contributions, you are agreeing that your contribution is made subject to the terms of our Contributor Ownership Statement