Test/resource mgt#48
Conversation
bacchuongdaungo
force-pushed
the
test/resource-mgt
branch
from
2f82982 to
fc2254d
Compare
lhmcgann
left a comment
lhmcgann
left a comment
There was a problem hiding this comment.
I didn't look as closely at the specific test cases or asserts yet, but will do so after edits are made for the few comments left in this review. Also, if you can doc-comment all your test classes and methods, as well as all the library classes, methods, properties, etc they're testing, that would be great! Some examples in the commit I added to this branch
| public int NumTotalResources => this.IdPool.NumUsedIds; | ||
| public int NumTotalResources => this.Resources.Count; | ||
|
|
||
| // Constructor |
There was a problem hiding this comment.
Make this a docstring comment, i.e., ///, etc
Also, include docstring comments for all classes, properties, fields, and methods
| // TODO: does Used check validity again? | ||
| return this.IdPool.IsValidId(globalId) && | ||
| this.IdPool.IsUsed(globalId); | ||
| this.IdPool.IsUsed(globalId) && this.Resources.ContainsKey(globalId); |
There was a problem hiding this comment.
Is this line needed? Was it failing before? the IdPool.IsValidID + IsUsed checks should be equivalent, so if it was failing before, something bigger is broken
There was a problem hiding this comment.
For the current implementation of IsValidId and IsUsed, it is crucial to have the line since we are checking for the resource. I have tried to implement the check in UseId but i can't do that without initiating an instance of ResourceManager, so I think it's simpler to have the check inside the RMgr class.
…within the min-max boundaries. -Adjust IncrementNumIds correspondingly. -Added unit test cases for constructor and IncrementNumId-Adjust TryAddResource to pass new test cases in ResourceManager.
IsWithinUpperBound Added test cases Replace int.MaxValue with Constants.POS_INFINITY Refix TryAddResource
2 participants
testcases for basic functionality and edge cases