python icon indicating copy to clipboard operation
python copied to clipboard

[Sattelite] Insufficient tests

Open tamireinhorn opened this issue 2 years ago • 9 comments

This exercise presents an interesting challenge, specially after solving Binary Search Trees. However, for some reason, its trees do not use numerical values, and even the letter values do not seem to follow the usual alphabetical logic as far as I can see. Coupling that with the fact that of the 7 tests, only 2 are not exception verifications, I think the exercise could have more tests, derived from the trees in the Binary Search Trees exercise. I've even written them in my repo for solutions.

tamireinhorn avatar Jun 14 '22 12:06 tamireinhorn

🤖   🤖

Hi! 👋🏽 👋 Welcome to the Exercism Python Repo!

Thank you for opening an issue! 🐍  🌈 ✨


  •   If you are requesting support, we will be along shortly to help. (generally within 72 hours, often more quickly).
  •   Found a problem with tests, exercises or something else??  🎉
      ◦ We'll take a look as soon as we can & identify what work is needed to fix it. (generally within 72 hours).

​          ◦ If you'd also like to make a PR to fix the issue, please have a quick look at the Pull Requests doc.
             We  💙  PRs that follow our Exercism & Track contributing guidelines!

  •   Here because of an obvious (and small set of) spelling, grammar, or punctuation issues with one exercise,
      concept, or Python document?? 🌟 Please feel free to submit a PR, linking to this issue. 🎉
    ‼️  Please Do Not ‼️

    ​        ❗ Run checks on the whole repo & submit a bunch of PRs.
                  This creates longer review cycles & exhausts reviewers energy & time.
                  It may also conflict with ongoing changes from other contributors.
    ​        ❗ Insert only blank lines, make a closing bracket drop to the next line, change a word
                  to a synonym without obvious reason, or add trailing space that's not an EOL for the very end of text files.         ❗ Introduce arbitrary changes "just to change things" .

            ...These sorts of things are not considered helpful, and will likely be closed by reviewers.

  • For anything complicated or ambiguous, let's discuss things -- we will likely welcome a PR from you.
  • Here to suggest a feature or new exercise?? Hooray! Please keep in mind Chesterton's Fence.
    Thoughtful suggestions will likely result faster & more enthusiastic responses from maintainers.

💛  💙  While you are here... If you decide to help out with other open issues, you have our gratitude 🙌 🙌🏽.
Anything tagged with [help wanted] and without [Claimed] is up for grabs.
Comment on the issue and we will reserve it for you. 🌈 ✨

github-actions[bot] avatar Jun 14 '22 12:06 github-actions[bot]

Hi @tamireinhorn 👋🏽

Thank you for filing this issue. I agree: the tests look thin for statellite, and it could benefit from more tests being added.

We pull our data for satellite from the problem-specifications repo, and auto-generate the tests you see here on the track.

So I would recommend that you PR your test cases to that repo, so that all tracks can discuss and benefit from them. Once the change is made in problems-specifications, we can pull it into the Python track and re-generate the test file for the exercise.

BethanyG avatar Jun 14 '22 16:06 BethanyG

If I understand correctly, I'd just have to update the following file ( https://github.com/exercism/problem-specifications/blob/main/exercises/satellite/canonical-data.json) right? In that case, what do I put as the uid for them?

On Tue, Jun 14, 2022 at 7:39 PM BethanyG @.***> wrote:

Hi @tamireinhorn https://github.com/tamireinhorn 👋🏽

Thank you for filing this issue. I agree: the tests look thin for statellite, and it could benefit from more tests being added.

We pull our data for satellite from the problem-specifications https://github.com/exercism/problem-specifications/blob/main/exercises/satellite/canonical-data.json repo, and auto-generate the tests you see here on the track.

So I would recommend that you PR your test cases to that repo, so that all tracks can discuss and benefit from them. Once the change is made in problems-specifications, we can pull it into the Python track and re-generate the test file for the exercise.

— Reply to this email directly, view it on GitHub https://github.com/exercism/python/issues/3114#issuecomment-1155439623, or unsubscribe https://github.com/notifications/unsubscribe-auth/AI5HUY5EK2VBHN234YLB5IDVPCYUDANCNFSM5YXSYXEA . You are receiving this because you were mentioned.Message ID: @.***>

tamireinhorn avatar Jun 14 '22 18:06 tamireinhorn

Hi @tamireinhorn,

That is correct -- you will want to update the canonical-data.json for the exercise. 😄

You can find the specifications for the file under the README -- Test Data. Any valid UUID(v4) will work. I often use this web-based UUID Generator to generate the ids, but you can use any method, as long as they're valid UUID(v4).

I ... think that's all that is needed, but make sure to read through the linked README, as I may have forgotten something.

BethanyG avatar Jun 14 '22 18:06 BethanyG

Here's the PR

tamireinhorn avatar Jun 15 '22 00:06 tamireinhorn

Still not enough reviews there, sadly

tamireinhorn avatar Jun 27 '22 10:06 tamireinhorn

My apologies @tamireinhorn - I've been really swamped. As soon as I get time to go back through the exercise, I will go ahead and add my review for your proposed changes. Please bear with me. 😄

BethanyG avatar Jul 27 '22 16:07 BethanyG

No worries, thanks for the update!

On Wed, Jul 27, 2022 at 7:13 PM BethanyG @.***> wrote:

My apologies @tamireinhorn https://github.com/tamireinhorn - I've been really swamped. As soon as I get time to go back through the exercise, I will go ahead and add my review for your proposed changes. Please bear with me. 😄

— Reply to this email directly, view it on GitHub https://github.com/exercism/python/issues/3114#issuecomment-1196964529, or unsubscribe https://github.com/notifications/unsubscribe-auth/AI5HUY5QRUUI4JHP62YB72DVWFN27ANCNFSM5YXSYXEA . You are receiving this because you were mentioned.Message ID: @.***>

tamireinhorn avatar Jul 28 '22 09:07 tamireinhorn

As a note the problems-specifications PR is still pending with a note from Glenn from 6 days ago about some additional test case scenarios and proposals.

BethanyG avatar Sep 13 '22 15:09 BethanyG

Closing this as there is an issue open in problem specifications.

BethanyG avatar Feb 25 '23 08:02 BethanyG