[Clock] Proposal for additional tests

[astrieanna]

I was mentoring a friend who solved the problem in a way that surprised me: his __repr__ implementation always returned the values the clock was initially constructed with. Regardless of how you mutated the clock, his __repr__ would stay the same. Since this incorrect solution passed all existing tests, I'd like to add a couple more tests.

Since the tests for __repr__ are in practice/clock/.meta/additional_tests.json, I'm assuming this repo is the right place to add some additional tests.

From looking at how the tests are generated, I'm not sure what the easiest approach would be. The way the current repr type tests are setup makes it easy to add "create a Clock, immediately call __repr__" tests, but adding a couple of tests that add/subtract before calling __repr__ would probably require a new type of test? (since it would need different treatment by the Jinja template).

I was also thinking about adding tests of __repr__ in addition to __str__ to the add and subtract tests (since those tests are why __str__ has to update), but that would require the Jinja template to reformat each hh:mm string from upstream into Clock(hh,mm) while removing the leading zeros. I'm not really familiar with Jinja, so that seems like it might be complicated to template correctly.

I'm happy to make a PR, but I'd like input first on how they should be added. :)

[github-actions]

🤖   🤖

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. 🌈 ✨

[BethanyG]

Hi @astrieanna 👋🏽

Apologies for the email spam -- there was something funky with my GH interface, and this message posted while I was still writing it. Nuked the first half of my message. 😱

Thank you so much for filing this issue - and I would be thrilled if you wanted to PR additional tests for the Clock exercise!

Let's take these point-by-point:

Since the tests for repr are in practice/clock/.meta/additional_tests.json, I'm assuming this repo is the right place to add some additional tests.

🌟 You are correct. We'd add these proposed tests in the additional_tests.json file, as they are specific to this track's implementation.

The way the current repr type tests are setup makes it easy to add "create a Clock, immediately call repr" tests, but adding a couple of tests that add/subtract before calling repr would probably require a new type of test? (since it would need different treatment by the Jinja template).

🤔 Yeah. That does sound like its a new (or at least slightly more complex) test type. No worries -- what I usually do (since I am by no means a JinJa expert) is :

• Take a copy of the generated test file and add the tests I am interested in to it in Python
• See if I can work my way backward into what parameters I could tag the JinJa customization off of.
  • For these scenarios, it sounds like a "make modifications to tine before calling repr or str" . That might mean having a key in additional_tests.json that's something like "combo" or "repr with mods", or "str with mods", etc.
  • I then would introduce the key, but not modify the rest of the format for the data (unless absolutely necessary), since it's sorta standardized across tests.
• Once I have a key or other thing I can "filter" on, I can then do a check/conditional logic in the template, create code for the scenario (maybe even a macro for this since its multiple tests), and have the tests generated accordingly.
• Once I do a template modification pass, I can then generate a new test file and compare the results to my manually made tests, to see if they match.
• Cycle till I get it done. Or quit in frustration (joking) 😄

I was also thinking about adding tests of repr in addition to str to the add and subtract tests (since those tests are why str has to update), but that would require the Jinja template to reformat each hh:mm string from upstream into Clock(hh,mm) while removing the leading zeros. I'm not really familiar with Jinja, so that seems like it might be complicated to template correctly.

So ... forgive me if I am mis-interpreting here. I see two things:

• Our addendum file already does this reformatting, and since tests are immutable, you'd be adding new tests in the addendum (in which case you could format them as needed). So you could add a test with a "add plus str" property, under a"description": "Add minutes", "cases": [] array. I am pretty sure that the test generator knows how to handle this and concatenate the tests accordingly. If not -- well, we have a bug to fix. Either way, worth trying.

• 🤔 Maybe reformatting things will be difficult in JinJa - but I've found that once I've gotten into a rhythm, many things in JinJa are surprisingly straightforward. Except for heavy conditional nesting -- that's super cruddy -- but reformatting a datetime string may not be so bad. I am pretty sure I have see reformatting macros. Keep in mind that you can also do things like put python code in to call datetime to reformat the string for you -- or even call Clock. When I get a chance, I will see if I can find a template that does that sort of thing. Happy to also troubleshoot with you or problem solve -- this templating stuff is intimidating, so maybe collaboration or a back-and-forth is in order?

Just let me know what I can do to help. I think the tests you propose would be super-helpful, and really support the addendums we've included in this exercise - so I'd love to see them happen. 😄

[BethanyG]

@astrieanna -- just a friendly ping! Are you still interested in working this? Just let me know...thanks!

[BethanyG]

Hi @astrieanna -- as noted in #3008, this exercise now has over 100 test cases. It feels like we should take a pause and figure out how to pare that down and then maybe add some cases as proposed here. To that end, I am going to close this issue for now, with the thought that we'd maybe revisit this entire exercise in a few months to see where we're at, and what we want to do. I'll log a "tracking issue" meanwhile. Let me know if you have any questions or issues.