-
-
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
Add Test for confirm to process #269
Add Test for confirm to process #269
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
Great improvement!
Most of my comments are related to the linear flow described in the testing strategy - being able to read the test top to bottom without having to scroll up to check for additional variable values etc. I could ask myself "what process is in the mockProcess
? Does the value itself matter?" and in order to find out I would have to scroll up to the setup.
In other words, repeating oneself is okay in tests. Explicit values increase the readability which is what truly matters.
packages/cli/src/lib/core/prompt/confirm-to-process.unit.test.ts
Outdated
Show resolved
Hide resolved
packages/cli/src/lib/core/prompt/confirm-to-process.unit.test.ts
Outdated
Show resolved
Hide resolved
packages/cli/src/lib/core/prompt/confirm-to-process.unit.test.ts
Outdated
Show resolved
Hide resolved
packages/cli/src/lib/core/prompt/confirm-to-process.unit.test.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Katka Pilátová <tlacencin@gmail.com>
Co-authored-by: Katka Pilátová <tlacencin@gmail.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Is this okey ? await confirmToProcess({
prompt: MOCK_PROMPT,
process: jest.fn(),
})(EMPTY_CONTEXT); |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
Nice!
ad your #269 (comment)
I would personally just hardcode a short prompt message everywhere as I wouldn't mind the repetition. But if you think it doesn't look good, it is fine like this as long as it is clear that the value is static.
☕ Order Coffee ☕Date/Time: 2024-03-05 16:47
🔒 Budgets🧭 Navigate to coffee cartResource Size Budget
Resource Count Budget
Timing Budget
🧭 Navigate to githubResource Size Budget
Resource Count Budget
Timing Budget
|
❗❗❗ report generated by this PR ❗❗❗☕ Order Coffee ☕Date/Time: 2024-03-05 16:48
🔒 Budgets🧭 Navigate to coffee cartResource Size Budget
Resource Count Budget
Timing Budget
🧭 Navigate to githubResource Size Budget
Resource Count Budget
Timing Budget
|
TL;DR;
This MR refactors the ask to skip functionality and adds proper unit test.
Description
This MR adds unit test to the askToSkip functionality while refactoring it to improve readability.