-
Notifications
You must be signed in to change notification settings - Fork 2
Test for doSeparatedBy #23
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: master
Are you sure you want to change the base?
Test for doSeparatedBy #23
Conversation
table := newTable | ||
| newTable newSize | | ||
newSize := (table isEmpty) ifTrue: [1] ifFalse: [2 * table size]. "Ensure it grows from 0" | ||
newTable := Array new: newSize. |
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 why the new size is not double because it can have an impact on the grow efficiency
collection add: 2. "Duplicate" | ||
collection add: 1. "Duplicate" | ||
|
||
"Verify that collection size is correct (duplicates should not be counted)" |
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.
Can you split this test in two?
One test testDuplicatedObjectsAreNotAdded
One test testDoSeparatedBy
emptyCollection remove: 42. "Removing from empty set" | ||
]) on: Error do: [ :ex | errorFlagEmpty := true. ]. | ||
self assert: errorFlagEmpty. "Ensure an error is raised for empty set" | ||
|
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.
Can you split the tests into two different ones.
One for the empty and non empty case.
CTSmallOrderedSet >> errorNotFound [ | ||
self error: 'Not found' | ||
] | ||
|
||
{ #category : #'private ' } | ||
{ #category : 'private ' } |
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.
Can you add a comment stating that the 0 is returned to indicate that the element is not found.
|
||
{ #category : 'testing' } | ||
CTSmallOrderedSetTest >> testErrorNotFound [ | ||
| collectionSet emptyCollection errorFlagEmpty errorFlagNonExistent errorFlagAlreadyRemoved | |
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.
Can you remove the unused temps?
The test covers the following:
@Ducasse, please review the PRs.
I have tried to cover all the missing tests you mentioned in the email.
Please let me if this works.
Thankyou!