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

High-Scores: add immutability test #1493

Closed
rpottsoh opened this issue Mar 30, 2019 · 11 comments
Closed

High-Scores: add immutability test #1493

rpottsoh opened this issue Mar 30, 2019 · 11 comments
Assignees

Comments

@rpottsoh
Copy link
Member

rpottsoh commented Mar 30, 2019

I have a student that has written method personalTopThree and allowed it to mutate the original input data. There are presently no tests to confirm that latest will return the same result before and after personalTopThree is called.

This is the case I am proposing:

    {
      "description": "latest not affected by personalTopThree",
      "property": "latest",
      "input": {
        "scores": [10, 30, 90, 30, 100, 20, 10, 0, 30, 40, 40, 70, 70, 25]
      },
      "expected": 25
    }

Is this a reasonable behavior to expect of personalTopThree?

@NobbZ
Copy link
Member

NobbZ commented Mar 30, 2019

As an Erlang maintainer and elixir mentor (who didn't did this exercise yet) what is mutation? ;)

@petertseng
Copy link
Member

It is true that unintended or unexpected mutation is a cause of many bugs.

You might find it useful to compare notes with the submitter of #1486. That pull request asks a similar question of "did the student implementation keep some sort of state, mutate this state when answering a query, and thus set itself up to give an incorrect answer to a subsequent query". The way I see it, you two want similar things and can operate under the principle of "two heads are better than one".

If I may ask a question, it seems that during the course of the test, you intend for both personalTopThree and latest messages to be sent? Do I have that right?
(I wouldn't mind you sketching out how you expect the test to be represented in an Exercism language of your choice or in pseudocode to illustrate)

If so, there will be the interesting question of how to represent in the JSON that this is what you intend to happen; currently that is done in the description I see.

@NobbZ
Copy link
Member

NobbZ commented Mar 30, 2019

This seems to be very implementation specific.

This test assumes, that we have an object that is created from some initial input.

As someone who does about 75% FP style programming only, I'm not used (anymore) to that kind of style.

Instead I'm used to just have functions, which gets passed the same input any time and therefore aren't subject to mutation.

Also in the canonical data, I do only see a call to latest, but not topThree, so assuming isolated tests, that create a new object under test each time, I'm unsure how this will change anything.

@rpottsoh
Copy link
Member Author

@NobbZ :

This seems to be very implementation specific.

Indeed it could very well be.

This test assumes, that we have an object that is created from some initial input.

This would be a more typical way to go about a solution in an OOP language. Certainly a more FP approach could be taken as well, which would make this issue moot I believe.

@petertseng :
I am well aware of #1486. This issue, #1493, didn't come to mind until I was reviewing a mentee's solution. I acknowledge that there are similarities between our two issues.

"two heads are better than one"

😃 perhaps. @jeffdparker, our two issues are similar. To continue the metaphor... maybe there is a way we can kill two birds with one stone... Any thoughts?

it seems that during the course of the test, you intend for both personalTopThree and latest messages to be sent? Do I have that right?

Yes, you do.

I am not sure how best to represent the intent of the test case in JSON. The description is fairly clear, but I concede not sufficiently clear.

An implementation might appear like so:

procedure THighScoresTest.latestNotAffectedByPersonalTopThree;
var
  Scores: IScores;
begin
  Scores := NewScores([10, 30, 90, 30, 100, 20, 10, 0, 30, 40, 40, 70, 70, 25]);
  Scores.personalTopThree;
  Assert.AreEqual(25, Scores.latest);
end;

The JSON maybe would look something like:

    {
      "description": "latest not affected by personalTopThree",
      "property": "latest",
      "input": {
        "scores": [10, 30, 90, 30, 100, 20, 10, 0, 30, 40, 40, 70, 70, 25],
        "operation": "personalTopThree"
      },
      "expected": 25
    }

Likely some definition (comment) of what "operation" means would need to be included, perhaps at the top of the canonical data file.

I do see that a test of this sort is probably not useful for a FP language but could be beneficial to a non-FP language.

@emcoding
Copy link
Contributor

I tend to think that adding this test is out of scope for this exercise. And that addressing the cases mentioned are better served as talking points in the mentoring.

The disadvantage I see in this test in this particular exercise (and it being a 'level 1' exercise) is that the student will get it right eventually to make the test pass, without the mentor knowing if the right solution is a 'trial and error' thing or a manifestation of understanding. As a Ruby mentor, it would be more work for me to find that out than addressing the topic when it's relevant in the given solution. (Which is not very often, in my experience in Ruby.)

On the other hand, this issue raises an interesting point if we address this concept explicitly enough in the Ruby track, and early enough. I'm going to look into that! Thanks for the inspiration :-)

I'd be interested to hear @pgaspar and @ErikSchierboom 's opinions.

@ErikSchierboom
Copy link
Member

ErikSchierboom commented Mar 31, 2019

I do see that a test of this sort is probably not useful for a FP language but could be beneficial to a non-FP language. -- rpottsoh

This hits the nail on the head. It is very similar to the discussion in this issue, where a test case is not applicable/useful for all language tracks. Maybe we could whatever we decide in #1487 and also use it here?

@pgaspar
Copy link
Member

pgaspar commented Apr 1, 2019

I agree this is a tricky question.

There's a proposal for marking some tests as "optional" currently being discussed on #1492. Could this test be a candidate for that optional flag?

That being said, I'm also hesitant about this test because there's no defined standard for how to represent these types of tests on the canonical JSON...

@sshine
Copy link
Contributor

sshine commented Apr 1, 2019

@pgaspar wrote:

Could this test be a candidate for that optional flag?

Whether a test is optional or a property are different beasts. To quote @coriolinus in #1225 (comment):

All property based test frameworks with which I am familiar (1, 2, 3) have a programmatic interface. Expressing a property based test suitable for multiple languages via a JSON file sounds would be an ambitious undertaking. I suspect that for our cases, we'll do better to construct function-based tests for the canonical data, and write comments in English about particular property-based tests which tracks may wish to implement manually.

I would encourage to

  1. view optional testing and immutability / other property-based tests as separate concerns,
  2. move the principal discussion of formalizing property tests to Schema optimises for testing f(inputs) = output. What of other types of tests? #1225,
  3. move the principal discussion of optional tests to Proposal: Add flag to mark test cases as optional to canonical schema #1492,
  4. sync or merge the discussion here or its outcome with that in High-Scores: Add immutability test #1486.

@sshine
Copy link
Contributor

sshine commented Jun 26, 2019

In the interest of finalizing or narrowing down new candidates for schema keys in #1496, so that at least #1492 can be merged, I would like to reiterate my previous comment and propose that we do not extend the schema such that these tests can be expressed:

You could either see them as property tests, in which case my previous quotation of @coriolinus applies. Or you could see them as small integration tests, in which case the quotation can be applied exactly as well.

If we are to have project-wide property/integration tests, we should have a better domain-specific language for specifying them. One that supports a language-agnostic, programmatic interface.

@F3PiX wrote:

I tend to think that adding this test is out of scope for this exercise. And that addressing the cases mentioned are better served as talking points in the mentoring.

Perhaps! I would be content to have them as track-specific tests.

If a track uses generators, make it compatible with supplying track-specific tests that are not auto-generated.

@rpottsoh: Do you find this reasonable for now?

@rpottsoh
Copy link
Member Author

Yes, for now.

@ErikSchierboom
Copy link
Member

This has been implemented in #1767

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants