ruby icon indicating copy to clipboard operation
ruby copied to clipboard

Clock - tests should add `eql?` and `hash`

Open joshgoebel opened this issue 6 years ago • 13 comments

Mentor notes mention these being important. If we want to teach them, why not add some final tests to make sure they work so students are aware of them?

joshgoebel avatar Mar 01 '19 18:03 joshgoebel

I think @iHiD's comments on Slack are relevant here:

Good question. Maybe because they tightly specify how things should be solved? Or because they're not auto-generated from the problem spec because they're Ruby specific.

@exercism/ruby In your experience, how important are these two arguments? I can see a future where many exercises start deviating from the problem specifications to have slightly different tests and I'm afraid that's a bit of a pain to maintain.

pgaspar avatar Mar 02 '19 03:03 pgaspar

I can see a future where many exercises start deviating from the problem specifications to have slightly different tests and I'm afraid that's a bit of a pain to maintain.

We're already there from what I've seen. But it's not unreasonable that some languages are going to have some things that don't map well to other languages. Seems the problem specs would need to allow for that.

joshgoebel avatar Mar 03 '19 16:03 joshgoebel

It is my opinion that the tests should not unduly influence (and surely not dictate) the implementation.

I can think of a requirement that "Shoes should remain on feet during normal activity" and a test of some activity checking that shoes are secured to feet should pass, but how those shoes are secured should not matter. If the student decides that a square knot works, then perhaps we should check that the shoes can still be removed. The student solution of using superglue perhaps passes the first test, while not passing the second test (at least in a timely manner). These things come out in the mentoring... what isn't tested for is the (perhaps standard) overhand -> bow knot., allowing for a "Hook and Loop" (similar to Velcro but different enough to not anger the patent gods) implementation to be used as well.

kotp avatar Mar 03 '19 16:03 kotp

Not sure I follow. == and eql? are both accepted Ruby conventions for equality in general, no? I don't think this is a matter where the student has a lot of choice in implementation. Of course the HOW is always up to them, but whether to implement or not is less so (if they want to be a proper Ruby citizen and "just work" out of the box).

joshgoebel avatar Mar 04 '19 13:03 joshgoebel

In your experience, how important are these two arguments? I can see a future where many exercises start deviating from the problem specifications to have slightly different tests and I'm afraid that's a bit of a pain to maintain.

That's a real problem. Especially if we want to add some changes that are made in the ProbSpec later on, but don't want to loose our special cases. It will be impossible to keep track of what we want and what we don't. The core team is aware of this problem, BTW, and thinking about solutions.

That doesn't help for Josh's question. And I'm not knowledgeable enough on Clock / case equality to see if this really is an implementation detail - the current Exercism policy is: don't test implementation - or something else. But, either way: @yyyc514 can we solve the student awareness by adding a link about case equality to the Ruby Readme? Which a probable nice side effect that it might make mentoring easier.

emcoding avatar Mar 04 '19 16:03 emcoding

But, either way: @yyyc514 can we solve the student awareness by adding a link about case equality to the Ruby Readme?

Does anyone read that stuff?

don't test implementation

Except that the tests themselves enforce a very specific API already for every exercise. I don't see why saying "your implementation of a class in Ruby should follow common conventions" - if the whole exercise is about just that - and the mentor notes say that's what you should be doing. I think you could make the case that testing eql? itself is NOT testing implementation at all.

add some changes that are made in the ProbSpec later on, but don't want to loose our special cases.

Isn't this why all sorts of things use magic comments?

# --- ### DON'T CHANGE ABOVE THIS LINE, AUTO GENERATED BY...
# BELOW THIS YOU CAN PUT CUSTOM TESTS THAT CANNOT BE AUTO GENERATED
# THAT WILL NOT BE DESTROYED BY THE AUTOGENERATOR

# --- ### END CUSTOM TESTS

You get the idea. :-)

joshgoebel avatar Mar 04 '19 16:03 joshgoebel

I'd appreciate @kytrinyx's opinion here - just to set us some guidance on the general principles for those of us who are ignorant (e.g. me 🙂 )

iHiD avatar Mar 20 '19 14:03 iHiD

The only implementation the current tests enforce are new, to_s and == and I think this is sufficient to solve the problem.

People are allowed to take the provided test file and add their own tests.

If someone wants to add tests for eql? and hash to their own test suite they are quite welcome to, but I don't think that these add anything to the base problem specified in the readme.

Meta note: It's a good idea to add links to the things you're discussing: https://github.com/exercism/ruby/blob/master/exercises/clock/README.md https://github.com/exercism/ruby/blob/master/exercises/clock/clock_test.rb

I don't know where the mentor notes are?

Insti avatar Mar 21 '19 21:03 Insti

https://github.com/exercism/website-copy/blob/master/tracks/ruby/exercises/clock/mentoring.md

Quoting:

A good solution should have the following:

  • Implement value-object semantics (immutability, ==/hash/eql? by value)

So if that's true, why not lead students there with tests?

joshgoebel avatar Mar 22 '19 00:03 joshgoebel

So if that's true, why not lead students there with tests?

It's an extension to the specified problem, and thus does not belong in the initial test suite.

But, I'm not convinced that is true.

Two clocks that represent the same time should be equal to each other.

Note 'equal to' rather than 'identical'

Insti avatar Mar 23 '19 20:03 Insti

I don't have a strong opinion in either direction. I personally see the Clock as a typical value object, where it doesn't matter which instance you have, if they tell the same time, then they're the same thing. That said, I could easily imagine a world where some clocks are fancy and others are not, and that they'd look different, feel different, and cost different amounts, where their identity actually would matter.

kytrinyx avatar Mar 24 '19 18:03 kytrinyx

Hm, it sounds like there are several questions here:

  1. Is any clock functionally equivalent to another? (see the question about "identity" that @Insti poses above)
  2. If they are functionally equivalent and identity doesn't matter, are the Ruby .eql? and .hash semantics important to the exercise?
  3. If these are important to the exercise, should we specify the .eql? and .hash API as part of the tests or not.

I don't have a strong enough opinion on any of these questions to guide. I think it's very important that we don't push people towards a specific implementation, especially if that implementation has elements that are incidental to the problem at hand.

kytrinyx avatar Mar 24 '19 18:03 kytrinyx

@iHiD this exercise in Ruby, I have mentored toward modelling clocks as they are represented in real life. When you change the time on your watch, you don't just get a new watch, you simply modify the state of your watch. Two watches may or may not be equal to each other (I have a clock on the wall that is equal to my watch two times a day!) but they are probably not identical. And they surely aren't the same object. The tests used to allow both approaches, and now seems to bend toward an implementation where we get a new clock just by adjusting/correcting the time for that clock.

There are other exercises where identity objects may focused on as well, and probably exclusively.

kotp avatar Mar 25 '19 14:03 kotp

I'm going to make a call here, and say that we will not implement .eql? and .hash for the clock. I like the simplicity of the exercise as is, and if we want to explore object equality from an immutability and identity perspective we can make up some new exercises for this.

kytrinyx avatar Oct 13 '22 10:10 kytrinyx