Add more tests to Satellite

[tamireinhorn]

As discussed on #3114 on the Python Track, the Sattelite exercise has very few tests, and the trees used in the example don't even seem to follow an understandable logic for comparison, thus making the exercise not as clear to the student. I've thus added a few tests based on the Binary Search Tree exercise.

[petertseng]

I'd like to discuss the value of having the values be strings that happen to contain numbers. Since these are just strings and the contents of them are unimportant, it doesn't seem to matter or be necessary to add tests with such strings.

If these tests catch some class of mistake that students usually make in their implementations, let's hear what those mistakes are and see what to do from there.

If the goal of these tests is just more coverage for different code paths that might arise when constructing the trees, I would suggest it's better to use letters as the other tests do; to introduce numbers when they are not the point of the test would obscure the point of the test and make them harder for students to understand.

We should also think about test ordering. Simpler tests first, I'd think.

[tamireinhorn]

Oops, I think I sort of mishandled the values in itself, sorry about that. The issue here is that the concept of a Binary Search Tree requires values that are comparable, and as far as I could tell, the trees used didn't use lexicographic order. Using the same examples as the BST exercise ties the concepts together in a neat way, and allows the student to have a proper understanding of why the tree is like that and thus figure the solution to the problem. Since there are just two examples of trees, it is entirely possible that the student's solution does not apply to a different BST, and hence we need more tests, but I fear that the arbitrary values detract from the understanding, as it did for me. Em qua, 15 de jun de 2022 08:04, Peter Tseng ***@***.***> escreveu:

…

I'd like to discuss the value of having the values be strings that happen to contain numbers. Since these are just strings and the contents of them are unimportant, it doesn't seem to matter or be necessary to add tests with such strings. If these tests catch some class of mistake that students usually make in their implementations, let's hear what those mistakes are and see what to do from there. If the goal of these tests is just more coverage for different code paths that might arise when constructing the trees, I would suggest it's better to use letters as the other tests do; to introduce numbers when they are not the point of the test would obscure the point of the test and make them harder for students to understand. We should also think about test ordering. Simpler tests first, I'd think. — Reply to this email directly, view it on GitHub <#2058 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AI5HUY3XKGBNG3PCFTHK27DVPFP6TANCNFSM5YZOYT2Q> . You are receiving this because you authored the thread.Message ID: ***@***.***>

the new test cases are correct

[jiegillet]

I'm all for having more test cases for this exercise, but I also don't see the point of introducing integers.
If they are meant to be treated as strings, nothing changes other than the possibility of adding "confusing" cases such as "100" being smaller than "2".
If they are meant to be treated as actual integers, this would possibly complicate things for some typed languages that would have to keep the input polymorphic, and that is not the point of the exercise.

Please also mind @petertseng's advice about ordering the exercises in order of difficulty.

Other than that, I'll happily approve.

[tamireinhorn]

@jiegillet Upon revisiting this, I'm 100% with you. Integers are unnecessary to the exercise, and I think we could just have more varied examples with letters just like the cases we already have, because just 2 exercises is not much.

[glennj]

I think we also need a test where the validations succeed initially, but fail further down the tree:

This input is the existing "Tree with many items" data with the last 2 preorder elements swapped:

      {
        "preorder": ["a", "i", "x", "r", "f"],
        "inorder": ["i", "a", "f", "x", "r"]
      }

At the top level we have this which passes all the assertions

{
  "v": "a",
  "l": build_tree({"preorder": ["i"], "inorder": ["i"]}),
  "r": build_tree({"preorder": ["x", "r", "f"], "inorder": ["f", "x", "r"]})
}

When we get to building the right child, rooted at "x", we have

{
  "v": "x",
  "l": build_tree({"preorder": ["r"], "inorder": ["f"]}),
  "r": build_tree({"preorder": ["f"], "inorder": ["r"]})
}

Those recursive calls should fail the "traversals must have the same elements" assertion.