-
Notifications
You must be signed in to change notification settings - Fork 141
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
solution #117
base: master
Are you sure you want to change the base?
solution #117
Conversation
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.
Hey, I've left some comments to improve the tests.
src/fillTank.test.js
Outdated
it(`should ordered full tank if amount is not given`, () => { | ||
const customer = { | ||
money: 3000, | ||
vehicle: { | ||
maxTankCapacity: 40, | ||
fuelRemains: 10, | ||
}, | ||
}; | ||
|
||
fillTank(customer, 5); | ||
|
||
expect(customer).toEqual({ | ||
money: 2850, | ||
vehicle: { | ||
maxTankCapacity: 40, | ||
fuelRemains: 40, | ||
}, | ||
}); | ||
}); |
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.
The test named should ordered full tank if amount is not given does not actually handle the "full tank" scenario if no amount is passed. It calls fillTank(customer, 5)
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.
I do not understand you, default value for amount is Infinity
src/fillTank.test.js
Outdated
it(`should round the poured amount by discarding | ||
number to the tenth part`, () => { | ||
const customer = { | ||
money: 3000, | ||
vehicle: { | ||
maxTankCapacity: 40, | ||
fuelRemains: 8, | ||
}, | ||
}; | ||
|
||
fillTank(customer, 5, 4.67); | ||
|
||
expect(customer).toEqual({ | ||
money: 2977, | ||
vehicle: { | ||
maxTankCapacity: 40, | ||
fuelRemains: 12.6, | ||
}, | ||
}); | ||
}); | ||
|
||
it(`should round the price of the purchased fuel | ||
the to the nearest hundredth part.`, () => { | ||
const customer = { | ||
money: 3000, | ||
vehicle: { | ||
maxTankCapacity: 40, | ||
fuelRemains: 8, | ||
}, | ||
}; | ||
|
||
fillTank(customer, 5.12, 4.6); | ||
|
||
expect(customer).toEqual({ | ||
money: 2976.45, | ||
vehicle: { | ||
maxTankCapacity: 40, | ||
fuelRemains: 12.6, | ||
}, | ||
}); | ||
}); |
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.
Multiple test cases are similar or overlapping in their goals. For example, should round the poured amount by discarding number to the tenth part and should round the price of the purchased fuel to the nearest hundredth part could be combined into a more holistic test if the goal is to check rounding behavior.
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 😃 just one last thing to change: combine the tests that check if rounding works properly. and add test that checks negative numbers
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.
Good job!
No description provided.