-
Notifications
You must be signed in to change notification settings - Fork 10
ILIA_BUBNOV-w1-UsingAPIs #34
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
## Test Summary | ||
|
||
**Mentors**: For more information on how to review homework assignments, please refer to the [Review Guide](https://github.com/HackYourFuture/mentors/blob/main/assignment-support/review-guide.md). | ||
|
||
### 3-UsingAPIs - Week1 | ||
|
||
| Exercise | Passed | Failed | ESLint | | ||
|-----------------------|--------|--------|--------| | ||
| ex1-johnWho | 9 | - | ✓ | | ||
| ex2-checkDoubleDigits | 11 | - | ✓ | | ||
| ex3-rollDie | 7 | - | ✓ | | ||
| ex4-pokerDiceAll | 7 | - | ✓ | | ||
| ex5-pokerDiceChain | 5 | - | ✓ | |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,9 +27,13 @@ exercise file. | |
import { rollDie } from '../../helpers/pokerDiceRoller.js'; | ||
|
||
export function rollDice() { | ||
// TODO Refactor this function | ||
const dice = [1, 2, 3, 4, 5]; | ||
return rollDie(1); | ||
const promises = dice.map(num => { | ||
return new Promise((resolve, reject) => { | ||
rollDie(num).then(resolve).catch(reject); | ||
}) | ||
}) | ||
return Promise.all(promises); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While this code works, it adds unnecessary redundancy and complexity since the You could also leave out the extra promise and do
|
||
} | ||
|
||
function main() { | ||
|
@@ -43,4 +47,7 @@ if (process.env.NODE_ENV !== 'test') { | |
main(); | ||
} | ||
|
||
// TODO Replace this comment by your explanation that was asked for in the assignment description. | ||
/* | ||
I think that the rolls are happening inside the API and multiple Promises are running which causes the outputs to mix up when they go to | ||
in the Event Queue and then end up in the Call Stack | ||
*/ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you got it right, although you chose a rather technical language to explain it. It could also be summarized as:
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Your solution is good, and the benefit is that it's easy to understand what's happening or troubleshoot. On the other hand, it includes some code repetition and would be harder to scale if you had a larger number of dice to throw. If you wanted to streamline the code, you could define a function factory that returns a new
Then, you could do:
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
*** Unit Test Error Report *** | ||
|
||
PASS .dist/3-UsingAPIs/Week1/unit-tests/ex1-johnWho.test.js | ||
api-wk1-ex1-johnWho | ||
✅ should exist and be executable (1 ms) | ||
✅ should have all TODO comments removed | ||
✅ `getAnonName` should not contain unneeded console.log calls (1 ms) | ||
✅ should call `new Promise()` | ||
✅ should take a single argument | ||
✅ `resolve()` should be called with a one argument | ||
✅ `reject()` should be called with a one argument | ||
✅ should resolve when called with a string argument (1 ms) | ||
✅ should reject with an Error object when called without an argument (1 ms) | ||
|
||
Test Suites: 1 passed, 1 total | ||
Tests: 9 passed, 9 total | ||
Snapshots: 0 total | ||
Time: 0.832 s | ||
Ran all test suites matching /C:\\Users\\BidonLoverXXX\\WebstormProjects\\Assignments-cohort52\\.dist\\3-UsingAPIs\\Week1\\unit-tests\\ex1-johnWho.test.js/i. | ||
No linting errors detected. | ||
No spelling errors detected. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
*** Unit Test Error Report *** | ||
|
||
PASS .dist/3-UsingAPIs/Week1/unit-tests/ex2-checkDoubleDigits.test.js | ||
api-wk1-ex2-checkDoubleDigits | ||
✅ should exist and be executable (2 ms) | ||
✅ should have all TODO comments removed | ||
✅ `checkDoubleDigits` should not contain unneeded console.log calls (1 ms) | ||
✅ should call new Promise() | ||
✅ `resolve()` should be called with a one argument | ||
✅ `reject()` should be called with a one argument | ||
✅ should be a function that takes a single argument | ||
✅ (9) should return a rejected promise with an Error object | ||
✅ (10) should return a promise that resolves to "This is a double digit number!" (1 ms) | ||
✅ (99) should return a promise that resolves to "This is a double digit number!" (1 ms) | ||
✅ (100) should return a rejected promise with an Error object | ||
|
||
Test Suites: 1 passed, 1 total | ||
Tests: 11 passed, 11 total | ||
Snapshots: 0 total | ||
Time: 0.689 s | ||
Ran all test suites matching /C:\\Users\\BidonLoverXXX\\WebstormProjects\\Assignments-cohort52\\.dist\\3-UsingAPIs\\Week1\\unit-tests\\ex2-checkDoubleDigits.test.js/i. | ||
No linting errors detected. | ||
No spelling errors detected. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
*** Unit Test Error Report *** | ||
|
||
PASS .dist/3-UsingAPIs/Week1/unit-tests/ex3-rollDie.test.js | ||
api-wk1-ex3-rollDie | ||
✅ should exist and be executable (1 ms) | ||
✅ should have all TODO comments removed | ||
✅ should call `new Promise()` (1 ms) | ||
✅ `resolve()` should be called with a one argument | ||
✅ `reject()` should be called with a one argument | ||
✅ should resolve when the die settles successfully (1 ms) | ||
✅ should reject with an Error when the die rolls off the table (1 ms) | ||
|
||
Test Suites: 1 passed, 1 total | ||
Tests: 7 passed, 7 total | ||
Snapshots: 0 total | ||
Time: 0.814 s, estimated 1 s | ||
Ran all test suites matching /C:\\Users\\BidonLoverXXX\\WebstormProjects\\Assignments-cohort52\\.dist\\3-UsingAPIs\\Week1\\unit-tests\\ex3-rollDie.test.js/i. | ||
No linting errors detected. | ||
No spelling errors detected. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
*** Unit Test Error Report *** | ||
|
||
(node:72180) TimeoutNegativeWarning: -5 is a negative number. | ||
Timeout duration was set to 1. | ||
(Use `node --trace-warnings ...` to show where the warning was created) | ||
PASS .dist/3-UsingAPIs/Week1/unit-tests/ex4-pokerDiceAll.test.js | ||
api-wk1-ex4-pokerDiceAll | ||
✅ should exist and be executable (2 ms) | ||
✅ should have all TODO comments removed (1 ms) | ||
✅ `rollDice` should not contain unneeded console.log calls | ||
✅ should use `dice.map()` | ||
✅ should use `Promise.all()` | ||
✅ should resolve when all dice settle successfully (9 ms) | ||
✅ should reject with an Error when a die rolls off the table (86 ms) | ||
|
||
Test Suites: 1 passed, 1 total | ||
Tests: 7 passed, 7 total | ||
Snapshots: 0 total | ||
Time: 1.004 s | ||
Ran all test suites matching /C:\\Users\\BidonLoverXXX\\WebstormProjects\\Assignments-cohort52\\.dist\\3-UsingAPIs\\Week1\\unit-tests\\ex4-pokerDiceAll.test.js/i. | ||
No linting errors detected. | ||
No spelling errors detected. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
*** Unit Test Error Report *** | ||
|
||
(node:66272) TimeoutNegativeWarning: -5 is a negative number. | ||
Timeout duration was set to 1. | ||
(Use `node --trace-warnings ...` to show where the warning was created) | ||
PASS .dist/3-UsingAPIs/Week1/unit-tests/ex5-pokerDiceChain.test.js | ||
api-wk1-ex5-pokerDiceChain | ||
✅ should exist and be executable (1 ms) | ||
✅ should have all TODO comments removed | ||
✅ `rollDice` should not contain unneeded console.log calls (1 ms) | ||
✅ should resolve when all dice settle successfully (23 ms) | ||
✅ should reject with an Error when a die rolls off the table (95 ms) | ||
|
||
Test Suites: 1 passed, 1 total | ||
Tests: 5 passed, 5 total | ||
Snapshots: 0 total | ||
Time: 0.821 s | ||
Ran all test suites matching /C:\\Users\\BidonLoverXXX\\WebstormProjects\\Assignments-cohort52\\.dist\\3-UsingAPIs\\Week1\\unit-tests\\ex5-pokerDiceChain.test.js/i. | ||
No linting errors detected. | ||
No spelling errors detected. |
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.
Regarding your comment about missing
return
statements in the original callback function, I didn't create this exercise, but I believe that that's the reason why the function erroneously returns both the Error and the Success callback if there are more than six rolls. The function execution is not stopped after the Error callback so it continues running till it reaches the number of originally scheduled rolls.