High-Scores: Add immutability test #1486

jeffdparker · 2019-03-24T14:19:22Z

I don't know the syntax, but would like to add a test that asks for top three (causing a sort) followed by a latest to see if the program sorted in place.

exercises/high-scores/canonical-data.json

rpottsoh · 2019-03-24T14:46:06Z

This proposed addition seems similar to what was removed in #1459.

jeffdparker · 2019-03-24T14:48:32Z

Sorry to have overlooked this history. I am back to request a test that calls latest() twice, due to a solution that used pop() to implement it.

rpottsoh · 2019-03-24T14:58:53Z

No problem. Can you post the code that has driven you to want a test added? It may be that the test need only be added for that track. But lets see what you have. I don't quite follow your reasoning for a new case at this point.

jeffdparker · 2019-03-24T16:41:02Z

I can't find it in my lists, but briefly the solution stores the results in a list, and then calls pop(), which removes and returns the last item on the list.

Like the issue with sorting, this method destroys the integrity of the data.

sshine · 2019-03-25T08:32:46Z

This does sound like you want a track-specific test.

The current canonical data describes the expected output of running functions in isolation. So if you'd like to test that a function is immutable, our current canonical data format assumes them to be and doesn't really support this explicitly.

Also, I don't think it should. If mentors/maintainers of other tracks disagree, feel free to object to this.

But I do think you should add a test for this in your track if this is something you expect to occur again.

emcoding · 2019-03-25T12:05:28Z

It looks like a perfect Talking Point to add to the Mentor Notes, rather than adding a test.

jeffdparker · 2019-03-25T16:49:57Z

There is already note about this in the python mentoring guide.

The problem of making sure the representation provides an accurate version of the state seems like the kind of thing that would be useful to test, especially as we try to automate what we can.

Is it difficult to make two calls inside a test? Is the reluctance to test this driven by technology or by pedagogy?

rpottsoh · 2019-03-26T04:22:14Z

Perhaps this would be acceptable:

       {
          "description": "Recent after a call to sort",
          "property": "latest",
          "input": {
            "personalTopThree": {
              "scores": [10, 20, 30, 40, 25]
            }
          },
          "expected": [25]
        }

The test would need to be last and probably shouldn't fall within the grouping of "Top 3 scores".

rpottsoh · 2019-03-26T04:41:17Z

My implementation of this test would look like:

procedure THighScoresTest.Recent_after_a_call_to_sort;
var
  Scores: IScores;
begin
  Scores := NewScores([10, 20, 30, 40, 25]);
  Scores := NewScores(Scores.personalTopThree.ToArray);
  Assert.AreEqual(25, Scores.latest);
end;

Only latest is tested.

petertseng · 2019-03-26T05:21:47Z

as you have seen, and as I will also now point you to #1225 to see, the schema optimises for f(input) = output; support for sending two (or more) messages to the same receiver was not given first-priority consideration in the design. If you would like to see some examples of how it was done, take a look at https://github.com/exercism/problem-specifications/blob/master/exercises/circular-buffer/canonical-data.json . Possibly react and zipper as well.

this comment should not be taken as a recommendation (either positive or negative) on whether this case should be added

ErikSchierboom · 2019-03-29T08:02:18Z

Also, I don't think it should. If mentors/maintainers of other tracks disagree, feel free to object to this.

I agree with @sshine. I understand the thoughts behind the test case, but I'm not in favor of adding it to the canonical data.

sshine · 2019-03-29T09:49:28Z

Is it difficult to make two calls inside a test?

No, but do you mean inside a test file for an exercise on a language track, or in canonical-data.json?

Is the reluctance to test this driven by technology or by pedagogy?

Technology, entirely!

My main point of objection would be that the schema of canonical-data.json is challenged up to a point where we wrap a bunch of human-readable sentences in JSON and rule out automated unit test generation.

So while some tracks may benefit from the added tests, other tracks for which automated test generation based on canonical-data.json can no longer run their test generator. This exercise was implemented by the following tracks: scala, ecmascript, delphi, nim, javascript, fsharp, csharp, python, ruby. According to exercism/discussions#155, scala, csharp and ruby have test generators.

I think your (property) test should be added to canonical-data.json.

But I'd prefer if canonical-data.json supported properties, then.

I suppose we could

Add a property test to high-scores' canonical-data.json that may break test generators, work on making canonical-data.json compatible with properties (one way or another, as continued in Schema optimises for testing f(inputs) = output. What of other types of tests? #1225), and then refactor this property test.
Pros: All tracks benefit from the extra test case. Test is stored in central location.
Cons: May break test generators. Format of canonical-data.json is abused.
Add track-specific tests to high-scores, work on making canonical-data.json compatible with properties (one way or another, as continued in Schema optimises for testing f(inputs) = output. What of other types of tests? #1225), add the property test to high-scores' canonical-data.json and sync the test version of the track(s) that added the local test case.
Pros: Doesn't break test generators. Keep format of canonical-data.json pure.
Cons: Track-specific tests don't propagate to other tracks. We may never resolve the discussion in Schema optimises for testing f(inputs) = output. What of other types of tests? #1225.
Something else entirely...

As a last note, I have to admit that I already abused canonical-data.json to store property-based tests in dnd-character; I reasoned that because this was a new exercise, it wouldn't break existing test generators, and because it operates on randomness, testing becomes very track-specific (mocking? injecting seed? property-based?), making test generation difficult if not impossible. But this still violates at least one argument I'm using against adding a property-based test to high-scores.

I'm not sure how we proceed from here. I withdraw my objection and am open to proceed in any way.

ErikSchierboom · 2022-01-05T14:24:25Z

@jeffdparker Are you still interested in working on this?

jeffdparker · 2022-01-05T15:57:14Z

Yes, I am interested. I’m out of town now, but will be back at my desk Monday Jeff Parker

On Wed, Jan 5, 2022 at 4:24 AM Erik Schierboom ***@***.***> wrote: @jeffdparker <https://github.com/jeffdparker> Are you still interested in working on this? — Reply to this email directly, view it on GitHub <#1486 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEHO6CLK3S4EBSWLXKKAOYLUURICJANCNFSM4HAWDTWA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

-- - Jeff Parker

ErikSchierboom · 2022-01-06T07:11:33Z

Great. Let's discuss what needs to happen when you're back.

kotp · 2022-01-24T03:13:12Z

Yes, I am interested. I’m out of town now, but will be back at my desk
Monday

@jeffdparker Which monday? ;)

jeffdparker · 2022-01-24T03:45:13Z

I am back, but I thought I saw that the bug was closed. I assume that what is needed is a new test: something like this def test_do_not_reorder(self): scores = [10, 30, 90, 30, 100, 20, 10, 0, 30, 40, 40, 70, 33] expected = 33 self.assertEqual(latest(scores), expected) expected = [100, 90, 70] self.assertEqual(personal_top_three(scores), expected) expected = 33 self.assertEqual(latest(scores), expected) expected = 100 self.assertEqual(personal_best(scores), expected) expected = 33 self.assertEqual(latest(scores), expected) I don't know how to insert such a test, but would be happy to learn jdp

…

On Sun, Jan 23, 2022 at 8:13 PM Victor Goff ***@***.***> wrote: Yes, I am interested. I’m out of town now, but will be back at my desk Monday @jeffdparker <https://github.com/jeffdparker> Which monday? ;) — Reply to this email directly, view it on GitHub <#1486 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEHO6CNMQ5NHI7EKCPFJ6RTUXS7VHANCNFSM4HAWDTWA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

glennj · 2022-01-24T14:52:45Z

This is what groovy does:

    def "Personal top three does not mutate"() {
        given:
        def hs = new HighScores(scores)
        def top3 = hs.personalTopThree()

        expect:
        hs.latest() == expected

        where:
        scores           || expected
        [40, 20, 10, 30] || 30
    }

But I don't know how to express that with the canonical-data-schema

jeffdparker · 2022-01-24T16:29:27Z

Glen If exercism is still using the same form of Unit tests, the test I provided above would work. It might be better to split it into to, to test best and top three separately def test_bet_does_not_reorder(self): scores = [10, 30, 90, 30, 100, 20, 10, 0, 30, 40, 40, 70, 33] expected = 33 self.assertEqual(latest(scores), expected) expected = 100 self.assertEqual(personal_best(scores), expected) expected = 33 self.assertEqual(latest(scores), expected) def test_top_does_not_reorder(self): scores = [10, 30, 90, 30, 100, 20, 10, 0, 30, 40, 40, 70, 33] expected = 33 self.assertEqual(latest(scores), expected) expected = [100, 90, 70] self.assertEqual(personal_top_three(scores), expected) expected = 33 self.assertEqual(latest(scores), expected)

…

On Mon, Jan 24, 2022 at 7:53 AM Glenn Jackman ***@***.***> wrote: This is what groovy does <https://github.com/exercism/groovy/blob/main/exercises/practice/high-scores/src/test/groovy/HighScoresSpec.groovy#L84> : def "Personal top three does not mutate"() { given: def hs = new HighScores(scores) def top3 = hs.personalTopThree() expect: hs.latest() == expected where: scores || expected [40, 20, 10, 30] || 30 } But I don't know how to express that with the canonical-data-schema — Reply to this email directly, view it on GitHub <#1486 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEHO6CPB54AY42X2ZYWETKLUXVRUZANCNFSM4HAWDTWA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

ErikSchierboom · 2022-01-25T11:42:01Z

@jeffdparker A separate PR added two test cases to verify immutability: https://github.com/exercism/problem-specifications/blob/main/exercises/high-scores/canonical-data.json#L87-L110 Could you check to see if those work for your suggestion too?

jeffdparker · 2022-01-25T16:47:18Z

Erik - Yes, those are the two test cases I'm concerned about. I see the language, but I don't see how that is translated into test cases. jdp PS Briefly, there are three calls: latest(), ..._best() and ..._top_three(). There is no temptation to modify the list in implementing latest(), but there is for the other two, and we should check that calling each does not modify the list, which we can measure by calling latest()

…

On Tue, Jan 25, 2022 at 4:42 AM Erik Schierboom ***@***.***> wrote: @jeffdparker <https://github.com/jeffdparker> A separate PR added two test cases to verify immutability: https://github.com/exercism/problem-specifications/blob/main/exercises/high-scores/canonical-data.json#L87-L110 Could you check to see if those work for your suggestion too? — Reply to this email directly, view it on GitHub <#1486 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEHO6CJULLKN5S5XFMJE5ADUX2EBVANCNFSM4HAWDTWA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

jeffdparker · 2022-02-03T17:11:50Z

Erik We are out of town: I will take a look at this later this weekend Jeff

On Tue, Feb 1, 2022 at 1:25 AM Erik Schierboom ***@***.***> wrote: I think ynfle's suggestion has one tiny bug, and that is that git push will fail after rebasing. Once you've rebased, you'll have to do git push --force. The fact that the contents in this PR are not updated makes sense as the push will have failed to update it. Could you try force pushing (note: this is generally discouraged, but necessary are rebasing a branch and pushing to an existing remote branch). — Reply to this email directly, view it on GitHub <#1486 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEHO6CMABKZX6UDDOMD3D23UY6KHRANCNFSM4HAWDTWA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

-- - Jeff Parker

jeffdparker · 2022-02-07T00:29:32Z

I'm looking at my system, and I see no trace of my changes or my pull request in the exercism branch. 1) I believe that I made the changes, but in a parallel repository, csci1, with the same problem statement. 2) If Erik thinks there is a problem, what does he think the solution is? Why would it fail after rebasing? 3) If I didn't make the changes to the exercism branch, do you have any advice on the next steps? jeff parker

…

On Thu, Feb 3, 2022 at 10:11 AM Jeff Parker ***@***.***> wrote: Erik We are out of town: I will take a look at this later this weekend Jeff On Tue, Feb 1, 2022 at 1:25 AM Erik Schierboom ***@***.***> wrote: > I think ynfle's suggestion has one tiny bug, and that is that git push > will fail after rebasing. Once you've rebased, you'll have to do git > push --force. The fact that the contents in this PR are not updated > makes sense as the push will have failed to update it. Could you try force > pushing (note: this is generally discouraged, but necessary are rebasing a > branch and pushing to an existing remote branch). > > — > Reply to this email directly, view it on GitHub > <#1486 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AEHO6CMABKZX6UDDOMD3D23UY6KHRANCNFSM4HAWDTWA> > . > Triage notifications on the go with GitHub Mobile for iOS > <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> > or Android > <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. > > You are receiving this because you were mentioned.Message ID: > ***@***.***> > -- - Jeff Parker

ErikSchierboom · 2022-02-08T08:20:26Z

I believe that I made the changes, but in a parallel repository, csci1,
with the same problem statement.

This is likely the problem.

If Erik thinks there is a problem, what does he think the solution is?
Why would it fail after rebasing?

That's git protecting you from overwriting remote changes with local changes. In essence: git rebase will have changed the history which means that git has no way of merging the remote changes with the local changes because they are two different trees.

If I didn't make the changes to the exercism branch, do you have any
advice on the next steps?

The branch for this PR is in this repo: https://github.com/jeffdparker/problem-specifications/tree/patch-2 The changes should be applied to the branch of this repo and then pushed.

jeffdparker · 2022-02-08T19:27:41Z

Thanks, Erik

…

On Tue, Feb 8, 2022 at 1:20 AM Erik Schierboom ***@***.***> wrote: 1. I believe that I made the changes, but in a parallel repository, csci1, with the same problem statement. This is likely the problem. 1. If Erik thinks there is a problem, what does he think the solution is? Why would it fail after rebasing? That's git protecting you from overwriting remote changes with local changes. In essence: git rebase will have changed the history which means that git has no way of merging the remote changes with the local changes because they are two different trees. 1. If I didn't make the changes to the exercism branch, do you have any advice on the next steps? The branch for this PR is in this repo: https://github.com/jeffdparker/problem-specifications/tree/patch-2 The changes should be applied to the branch of this repo and then pushed. — Reply to this email directly, view it on GitHub <#1486 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEHO6COXJS5UCUAC2YZJDJLU2DG5LANCNFSM4HAWDTWA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

Add tests for immutability after calling personal best Some solutions sort the array to find the largest. This breaks any subsequent call for recent scores, and isn't as fast as calling max. These two tests call recent and scores to verify that the scores have not changed.

jeffdparker · 2022-02-09T23:05:42Z

Erik - I have edited the file and pushed to patch-2. Let me know if more is needed. I'm curious to know how I could check the syntax before committing. I did it by eye, but would prefer some validation Jeff

…

On Tue, Feb 8, 2022 at 1:20 AM Erik Schierboom ***@***.***> wrote: 1. I believe that I made the changes, but in a parallel repository, csci1, with the same problem statement. This is likely the problem. 1. If Erik thinks there is a problem, what does he think the solution is? Why would it fail after rebasing? That's git protecting you from overwriting remote changes with local changes. In essence: git rebase will have changed the history which means that git has no way of merging the remote changes with the local changes because they are two different trees. 1. If I didn't make the changes to the exercism branch, do you have any advice on the next steps? The branch for this PR is in this repo: https://github.com/jeffdparker/problem-specifications/tree/patch-2 The changes should be applied to the branch of this repo and then pushed. — Reply to this email directly, view it on GitHub <#1486 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEHO6COXJS5UCUAC2YZJDJLU2DG5LANCNFSM4HAWDTWA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

Add missing commas

jeffdparker · 2022-02-09T23:18:56Z

Erik - I see that there is an automated check. There were a couple of commas missing - one in a prior checkin. I've tried to fix and recommit Jeff

…

On Tue, Feb 8, 2022 at 1:20 AM Erik Schierboom ***@***.***> wrote: 1. I believe that I made the changes, but in a parallel repository, csci1, with the same problem statement. This is likely the problem. 1. If Erik thinks there is a problem, what does he think the solution is? Why would it fail after rebasing? That's git protecting you from overwriting remote changes with local changes. In essence: git rebase will have changed the history which means that git has no way of merging the remote changes with the local changes because they are two different trees. 1. If I didn't make the changes to the exercism branch, do you have any advice on the next steps? The branch for this PR is in this repo: https://github.com/jeffdparker/problem-specifications/tree/patch-2 The changes should be applied to the branch of this repo and then pushed. — Reply to this email directly, view it on GitHub <#1486 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEHO6COXJS5UCUAC2YZJDJLU2DG5LANCNFSM4HAWDTWA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

ynfle · 2022-02-09T23:27:13Z

Hi @jeffdparker,

It seems that your push didn't fix it.

Check out https://github.com/exercism/problem-specifications#automated-tests for running at least some of the tests locally.
You can also checkout the GitHub Actions CI at https://github.com/exercism/problem-specifications/blob/main/.github/workflows/ci.yml if you're willing to dive in to that

Good luck

ynfle · 2022-02-09T23:28:29Z

It seems that property should be an array with [] around the 2 strings

ErikSchierboom

Just one comma missing I believe.

ErikSchierboom · 2022-02-10T07:25:15Z

exercises/high-scores/canonical-data.json

@@ -112,6 +112,30 @@
          },
          "expected": [25]
        }


There's a comma missing here.

ErikSchierboom · 2022-02-10T07:30:22Z

exercises/high-scores/canonical-data.json

+        },
+        {
+          "description": "Recent after a call to sort",
+          "property": "personalTopThree", "latest",


This is not valid JSON. Is this test case still needed? I sort of assumed that the two newer ones would handle this.

The property value must be a string. This looks like an array, but without the [ and ] characters.

jeffdparker · 2022-02-10T18:42:58Z

This is not valid JSON.

Sorry - what is the issue with this? Is it "scenarios"? Is it the two properties on the last line? Was that something I changed? It does not appear in the most recent diffs

Is this test case still needed? I sort of assumed that the two newer ones

would handle this. There are two issues: immutability after calling latest, and immutability after calling top three. The old test checked immutability after calling personal_best(), while the new tests check immutability after calling personal_top_three(). We should check immutability in both cases: you could argue that we don't need two tests to check the one thing. The tests come in pairs: one of these tests checks the most recent (last) element of the list. The other test in each pair asks for the full list, and checks that it is unchanged. The tests that use the latest score have the advantage that they don't need access beyond the API. Jeff

…

On Thu, Feb 10, 2022 at 12:30 AM Erik Schierboom ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In exercises/high-scores/canonical-data.json <#1486 (comment)> : > @@ -103,6 +103,38 @@ }, "expected": [30, 50, 20, 70], "scenarios": ["immutable"] + }, + { + "description": "Recent after a call to sort", + "property": "personalTopThree", "latest", This is not valid JSON. Is this test case still needed? I sort of assumed that the two newer ones would handle this. — Reply to this email directly, view it on GitHub <#1486 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEHO6CMAKIZVFERGUFNCCXDU2NSRRANCNFSM4HAWDTWA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

ErikSchierboom

The tests come in pairs: one of these tests checks the most recent (last)
element of the list. The other test in each pair asks for the full list,
and checks that it is unchanged. The tests that use the latest score have
the advantage that they don't need access beyond the API.

Correct, but the PR is now adding three tests. I think the first one should be removed, right?

ErikSchierboom · 2022-02-11T07:46:02Z

exercises/high-scores/canonical-data.json

+        },
+        {
+          "description": "Recent after a call to sort",
+          "property": "personalTopThree", "latest",


The property value must be a string. This looks like an array, but without the [ and ] characters.

jeffdparker · 2022-02-12T04:02:33Z

Agreed.

…

On Fri, Feb 11, 2022 at 12:48 AM Erik Schierboom ***@***.***> wrote: ***@***.**** commented on this pull request. The tests come in pairs: one of these tests checks the most recent (last) element of the list. The other test in each pair asks for the full list, and checks that it is unchanged. The tests that use the latest score have the advantage that they don't need access beyond the API. Correct, but the PR is now adding three tests. I think the first one should be removed, right? ------------------------------ In exercises/high-scores/canonical-data.json <#1486 (comment)> : > @@ -103,6 +103,38 @@ }, "expected": [30, 50, 20, 70], "scenarios": ["immutable"] + }, + { + "description": "Recent after a call to sort", + "property": "personalTopThree", "latest", The property value must be a string. This looks like an array, but without the [ and ] characters. — Reply to this email directly, view it on GitHub <#1486 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEHO6CLCELGQ7KW4IQZHRGLU2S5MZANCNFSM4HAWDTWA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

Removed redundant test that two property strings, rather than one "property": "personalTopThree", "latest",

jeffdparker · 2022-02-12T04:26:53Z

Erik - I removed the first test, as you suggested, in canonical-data.json I received an error message about the file, and chided me for not using prettier. I have downloaded npm and prettier, and run npx prettier -c canonical-data.json on my local file system. I am told All matched files use Prettier code style! Is there a better validation flag I should be using? jdp

…

On Fri, Feb 11, 2022 at 12:48 AM Erik Schierboom ***@***.***> wrote: ***@***.**** commented on this pull request. The tests come in pairs: one of these tests checks the most recent (last) element of the list. The other test in each pair asks for the full list, and checks that it is unchanged. The tests that use the latest score have the advantage that they don't need access beyond the API. Correct, but the PR is now adding three tests. I think the first one should be removed, right? ------------------------------ In exercises/high-scores/canonical-data.json <#1486 (comment)> : > @@ -103,6 +103,38 @@ }, "expected": [30, 50, 20, 70], "scenarios": ["immutable"] + }, + { + "description": "Recent after a call to sort", + "property": "personalTopThree", "latest", The property value must be a string. This looks like an array, but without the [ and ] characters. — Reply to this email directly, view it on GitHub <#1486 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEHO6CLCELGQ7KW4IQZHRGLU2S5MZANCNFSM4HAWDTWA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

jeffdparker · 2022-02-12T04:39:12Z

Erik - I had added the new tests as part of a case statement for the top-three API. I have moved the tests two braces out, and the system is silently dreaming up a new challenge. jdp

…

On Fri, Feb 11, 2022 at 12:48 AM Erik Schierboom ***@***.***> wrote: ***@***.**** commented on this pull request. The tests come in pairs: one of these tests checks the most recent (last) element of the list. The other test in each pair asks for the full list, and checks that it is unchanged. The tests that use the latest score have the advantage that they don't need access beyond the API. Correct, but the PR is now adding three tests. I think the first one should be removed, right? ------------------------------ In exercises/high-scores/canonical-data.json <#1486 (comment)> : > @@ -103,6 +103,38 @@ }, "expected": [30, 50, 20, 70], "scenarios": ["immutable"] + }, + { + "description": "Recent after a call to sort", + "property": "personalTopThree", "latest", The property value must be a string. This looks like an array, but without the [ and ] characters. — Reply to this email directly, view it on GitHub <#1486 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEHO6CLCELGQ7KW4IQZHRGLU2S5MZANCNFSM4HAWDTWA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

ErikSchierboom · 2022-02-12T12:14:58Z

/format

loziniak · 2022-02-15T09:57:37Z

seems already addressed by #1767 .

ErikSchierboom · 2022-02-15T10:17:05Z

seems already addressed by #1767 .

This is a new addition.

jeffdparker · 2022-02-15T16:45:07Z

seems already addressed by #1767

<#1767> . There are two calls in the API that need to be checked: this change checks the other one. As noted above:

Briefly, there are three calls: latest(), ..._best() and

..._top_three(). There is no temptation to modify the list in implementing latest(), but there is for the other two, and we should check that calling each does not modify the list...

…

On Tue, Feb 15, 2022 at 3:17 AM Erik Schierboom ***@***.***> wrote: seems already addressed by #1767 <#1767> . This is a new addition. — Reply to this email directly, view it on GitHub <#1486 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEHO6CL5X6Z372WF3UYN56TU3IR23ANCNFSM4HAWDTWA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

Add test

a293f94

I don't know the syntax, but would like to add a test that asks for top three (causing a sort) followed by a latest to see if the program sorted in place.

rpottsoh changed the title ~~Add test~~ High-Scores: Add test Mar 24, 2019

rpottsoh reviewed Mar 24, 2019

View reviewed changes

exercises/high-scores/canonical-data.json Outdated Show resolved Hide resolved

cmccandless mentioned this pull request Mar 27, 2019

high-scores: Added new testcase for data corruption exercism/python#1735

Closed

petertseng mentioned this pull request Mar 30, 2019

High-Scores: add immutability test #1493

Closed

cmccandless changed the title ~~High-Scores: Add test~~ High-Scores: Add immutability test Apr 1, 2019

This was referenced Apr 9, 2019

HighScore: Two new tests for methods interference exercism/python#1744

Closed

high-scores: incorrect solution can pass the tests exercism/python#1745

Closed

Base automatically changed from master to main January 27, 2021 15:31

jeffdparker added 2 commits February 9, 2022 15:53

Merge branch 'exercism:main' into patch-2

ee89fe8

Update canonical-data.json

5ce5ac2

Add missing commas

ErikSchierboom reviewed Feb 10, 2022

View reviewed changes

exercises/high-scores/canonical-data.json Outdated

@@ -112,6 +112,30 @@

},

"expected": [25]

}

Copy link

Member

ErikSchierboom Feb 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a comma missing here.

ErikSchierboom reviewed Feb 10, 2022

View reviewed changes

ErikSchierboom reviewed Feb 11, 2022

View reviewed changes

Update canonical-data.json

6bce49e

Removed redundant test that two property strings, rather than one "property": "personalTopThree", "latest",

[CI] Format code

7d1ae02

ErikSchierboom closed this Feb 15, 2022

ErikSchierboom reopened this Feb 15, 2022

ErikSchierboom approved these changes Feb 15, 2022

View reviewed changes

petertseng approved these changes Feb 15, 2022

View reviewed changes

kotp approved these changes Feb 15, 2022

View reviewed changes

kotp merged commit 88ce356 into exercism:main Feb 15, 2022

High-Scores: Add immutability test #1486

High-Scores: Add immutability test #1486

Conversation

jeffdparker commented Mar 24, 2019

rpottsoh commented Mar 24, 2019

jeffdparker commented Mar 24, 2019

rpottsoh commented Mar 24, 2019

jeffdparker commented Mar 24, 2019

sshine commented Mar 25, 2019 • edited Loading

emcoding commented Mar 25, 2019

jeffdparker commented Mar 25, 2019 • edited by rpottsoh Loading

rpottsoh commented Mar 26, 2019 • edited Loading

rpottsoh commented Mar 26, 2019 • edited Loading

petertseng commented Mar 26, 2019 • edited Loading

ErikSchierboom commented Mar 29, 2019

sshine commented Mar 29, 2019

ErikSchierboom commented Jan 5, 2022

jeffdparker commented Jan 5, 2022 via email

ErikSchierboom commented Jan 6, 2022

kotp commented Jan 24, 2022

jeffdparker commented Jan 24, 2022 via email

glennj commented Jan 24, 2022

jeffdparker commented Jan 24, 2022 via email

ErikSchierboom commented Jan 25, 2022

jeffdparker commented Jan 25, 2022 via email

jeffdparker commented Feb 3, 2022 via email

jeffdparker commented Feb 7, 2022 via email

ErikSchierboom commented Feb 8, 2022

jeffdparker commented Feb 8, 2022 via email

jeffdparker commented Feb 9, 2022 via email

jeffdparker commented Feb 9, 2022 via email

ynfle commented Feb 9, 2022

ynfle commented Feb 9, 2022

ErikSchierboom left a comment

Choose a reason for hiding this comment

ErikSchierboom Feb 10, 2022

Choose a reason for hiding this comment

ErikSchierboom Feb 10, 2022

Choose a reason for hiding this comment

ErikSchierboom Feb 11, 2022

Choose a reason for hiding this comment

jeffdparker commented Feb 10, 2022 via email

ErikSchierboom left a comment

Choose a reason for hiding this comment

ErikSchierboom Feb 11, 2022

Choose a reason for hiding this comment

jeffdparker commented Feb 12, 2022 via email

jeffdparker commented Feb 12, 2022 via email

jeffdparker commented Feb 12, 2022 via email

ErikSchierboom commented Feb 12, 2022

loziniak commented Feb 15, 2022

ErikSchierboom commented Feb 15, 2022

jeffdparker commented Feb 15, 2022 via email

sshine commented Mar 25, 2019 •

edited

Loading

jeffdparker commented Mar 25, 2019 •

edited by rpottsoh

Loading

rpottsoh commented Mar 26, 2019 •

edited

Loading

rpottsoh commented Mar 26, 2019 •

edited

Loading

petertseng commented Mar 26, 2019 •

edited

Loading