Skip to content
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

fixing failing test on Ternary Search Tree #355

Merged
merged 1 commit into from Jan 11, 2017

Conversation

pbodsk
Copy link
Contributor

@pbodsk pbodsk commented Jan 11, 2017

The problem was that the testdata generator could sometimes (but not on every test run...) generate keys that was not unique...which caused the find method to return the wrong values for keys when more keys existed in the tree with the same value.

the problem was that the testdata generator could generate keys that was not unique...which caused the `find` method to return the wrong values for keys when the keys were the same.
@vincentngo
Copy link
Contributor

I was wondering why the test were sometimes passing and sometimes failing randomly :D

let testCount = 30

func testCanFindStringInTree() {
var testStrings: [(key: String, data: String)] = []
let treeOfStrings = TernarySearchTree<String>()

for _ in (1...testCount) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pbodsk would it be possible to build the TST once and use that tree to perform your test? Instead of creating it on every test?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could extract testStrings and treeOfStrings into properties of XCTestCase, and perform a setup to generate the tree. That way each test focuses on what it's testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you point but...

The first test: testCanFindStringInTree, generates a tree containing String elements so therefore the testStrings and treeOfStrings are created there, like so:

    var testStrings: [(key: String, data: String)] = []
    let treeOfStrings = TernarySearchTree<String>()

The second test: testCanFindNumberInTree, generates a tree containing Int elements, so here the variables used looks like this:

    var testNums: [(key: String, data: Int)] = []
    let treeOfInts = TernarySearchTree<Int>()

(notice that one tree works on String elements and the other works on Int elements).

If all tests were using the same tree structure then I agree that it would be way better to generate/populate the treeOfStrings and the testStrings in setup(), but now the two tests are using different trees for their testing, and I think adding them as two different properties on the testcase would clutter the testclass.

If we add more tests at a later point then perhaps we should consider extracting treeOfStrings into a property and then do tests only with String elements.

Hope this makes sense to you (not trying to dodge the task here 😄)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok fair enough XD

@vincentngo vincentngo merged commit b9b491d into kodecocodes:master Jan 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants