Update of testcase generator to latest JSON format

[eklatzer]

Hey, as already discussed in Slack I have created a draft for the new version of the test case generator. I have also included the code covering #2055 (I have used code from #2276 and adjusted it a bit). Summary of what I have done:

• Collection of all test cases from the canonical-data.json and group by the value of the field property
• Removement of the test cases with inclue=false in the tests.toml
• Removement of the version info in the header of cases_test.go as the info is not provided in the canonical-data.json anymore (#1678)
• Generator expects now a struct for each of the groups defined by property (fields that have multiple data-types for one field have to be handled with an interface{} and have then to be handled by each specific generator)
• I have removed property.json as I think this is not needed anymore

To check if we have forgotten anything I have refactored the following generators:

• bowling: shows how multiple values for property can be handled to generate multiple cases with different structures and also how to handle an interface{} directly in the generator
• diamond: no special purpose, but I tried it as I made some changes to this one some days ago
• forth: shows how to handle an interface{} in the generator (I have also refactored the forth_test.go a bit as I don't think the info about the groups is really needed here and I also made use of t.Run and improved the error-messages)

NOTE: The use of multiple data types for one field mostly happens values and errors are used like here (expected is a struct and a number):

 {
      "uuid": "29220245-ac8d-463d-bc19-98a94cfada8a",
      "description": "an unstarted game cannot be scored",
      "property": "score",
      "input": {
        "previousRolls": []
      },
      "expected": {
        "error": "Score cannot be taken until the end of the game"
      }
    },
    {
      "uuid": "656ae006-25c2-438c-a549-f338e7ec7441",
      "description": "should be able to score a game with all zeros",
      "property": "score",
      "input": {
        "previousRolls": [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]
      },
      "expected": 0
    },

[github-actions]

Dear eklatzer

Thank you for contributing to the Go track on Exercism! 💙
You will see some automated feedback below 🤖. It would be great if you can make sure your PR covers those points. This will save your reviewer some time and your change can be merged quicker.

• ⬆️ The instructions and test cases for many practice exercises originate in the Exercism-wide problem-specifications repo. If the exercise you changed is listed there in the exercises folder, please consider the following.

  • Improvements to the instructions.md file should be made in the problem-spec repo so that all tracks can benefit.
    You can open a PR there instead.
  • If you want to add some language specific information, use the instructions.append.md file (see Practice Exercise Docs).
  • Test cases selected in the .meta/tests.toml file need to be implemented in the <exercise>_test.go file according to the specification in canonical-data.json.

• 🔗 If your PR fully fixes an issue, please include the text Fixes #issue_no in any line of the PR description. This will make the issue be automatically be closed when the PR is merged. If your PR is related to an existing issue but does not fix it completely, please link the issue anywhere in the description of the PR with #issue_no. You can read more about this in Github: Linking a pull request to an issue

• ✍️ If your PR is not related to an existing issue (and is not self-explaining like a typo fix), please make sure the description explains why the change you made is necessary.

• 🔤 If your PR fixes an easy to identify typo, if would be great if you could check for that typo in the whole repo. For example, if you found Unicdoe, use "replace all" in your editor (or command line magic) to fix it consistently.

Dear Reviewer/Maintainer

• 📏 Make sure you set the appropriate x:size label for the PR. (This also works after merging, in case you forgot about it.)

• 🔍 Don't be too nit-picky. If the PR is a clear improvement compared to the status quo, it should be approved as clear signal this is good to be merged even if the minor comments you might have are not addressed by the contributor. Further improvement ideas can be captured in issues (if important enough) and implemented via additional PRs.

• 🤔 After reviewing the diff in the "Files changed" section, take a moment to think about whether there are changes missing from the diff. Does something need to be adjusted in other places so the code or content stays consistent?

_{Automated comment created by PR Commenter 🤖.}

[eklatzer]

Hey @junedev and @andrerfcsantos, could you please have a look at the implementation and the changes on the test case generator and give me some feedback

[eklatzer]

Hey regarding #2056: I have done the following things:

• updated tests.toml
• created a new test-case generator
• implemented the tests of the generator

Here is what I faced: when using the code from the example.go to run the tests, all tests of the problem-specifications failed and all with the same problem. Here an example:
Input:

4 5 4
3 5 5
1 5 4

Expected output in problem-specifications:

 "expected": [
        {
          "row": 1,
          "column": 2
        },
        {
          "row": 2,
          "column": 2
        },
        {
          "row": 3,
          "column": 2
        }
      ]

-->[]Pair{{1, 2}, {2, 2}, {3, 2}

As this test was also implemented in the existing test-cases here the expected value there:

[]Pair{{0, 1}, {1, 1}, {2, 1}

As you can see each value is incremented by 1, compared to the problem-specifications (index instead of position). This is also the case for all other test cases. Possible solutions:

• we adjust the example solution to match the expected problem-specifications
• we increment each value of the expected result by 1, by using the following template:

{{printf "%d+1" .Row}},
{{printf "%d+1" .Column}},

Please tell me what you expect here

[andrerfcsantos]

Thank you so much for this! ❤️

Left some suggestions.

Other than the inline suggestions, two general comments about the PR:

• Right now CI is failing with this error:

tmp/lint/practice/saddle_points/cases_test.go:14: File is not `gofmt`-ed with `-s` (gofmt)
		[][]int{[]int{9, 8, 7}, []int{5, 3, 2}, []int{6, 6, 7}},

To fix this, in the root of the repository run gofmt -s -w ./...

• The Gen function is getting quite big and it's doing a lot. It might be worth separating it into several smaller functions that are more testable. However, I'm fine with this being a separate PR - that shouldn't block this one. Let me know what you think, and if you agree in making this a separate PR, I'll create the issue for it.

[andrerfcsantos]

we adjust the example solution to match the expected problem-specifications

I like this better, because it makes the student's solution match the position numbers provided in the example and in the tests.

It seems that this exercise originally had the table in the example using 0-based indexes, and when the exercise was introduced in the track, both description and tests were written with that assumption. However, with exercism/problem-specifications#1429 the description and canonical data were updated to use 1-based indexes in problem specs. Then with the change to v3, the exercises' descriptions for the Go track were updated to use the latest version by #1410, but the neither the tests or the exemplar were updated, so it seems a good idea to do it now.

[eklatzer]

Hey, is it OK to handle this add the end by adding 1 to the index-based values, or do we want to handle this somewhere else?

I removed the old tests, as the ones from problem-specifications have more test cases and cover similar problems.

[junedev]

Hey, is it OK to handle this add the end by adding 1 to the index-based values, or do we want to handle this somewhere else?

I removed the old tests, as the ones from problem-specifications have more test cases and cover similar problems.

@eklatzer I'm not sure I understand the question. If you are referring to how to handle this in the example solution then you are free to handle this however you like. The example solution does not have to be a very good solution, it only needs to be a solution that passes the tests for CI purposes.

[eklatzer]

Fine :) Are there any changes you want me to add?

[junedev]

Finished the review. Left some more minor comments/questions.

Two more general points (no need to fix everywhere here):

• When adding context for an error via fmt.Errorf or similar, the (%v) formatting you used is not idiomatic to Go. The old version with the colon is preferred. See for example https://go.dev/blog/go1.13-errors (section "Wrapping errors ..."). If you wonder why this is better, think about what happens when something is wrapped multiple times as it bubbles up the stack.
• I can recommend to make error wrapping a habit. At work, I barely ever return an error as is, I usually always wrap before returning it (see https://docs.gitlab.com/ee/development/go_guide/#adding-context for how to do it in way that you get reasonable output). Remember that by default, there are no stack traces attached to the errors so context is very important.

[junedev]

I don't understand why the value here is called "cache". Can you think of another name or add a comment to clarify?

[eklatzer]

I have changed the name and added a comment, but I am not sure if it is better now. What is your opinion?

[junedev]

Still a bit fuzzy but not a blocker.

[junedev]

Two more things I forgot to mention:

• THANKS! 🙂
• Please create an issue about updating/extending the README regarding generators. With this new system there are some things to be aware of when writing a generator that should be pointed out (e.g. that cases are flattened, that the key is the property).

[eklatzer]

Two more things I forgot to mention:

• THANKS! 🙂
• Please create an issue about updating/extending the README regarding generators. With this new system there are some things to be aware of when writing a generator that should be pointed out (e.g. that cases are flattened, that the key is the property).

Thanks for the feedback🙂

[eklatzer]

• The Gen function is getting quite big and it's doing a lot. It might be worth separating it into several smaller functions that are more testable. However, I'm fine with this being a separate PR - that shouldn't block this one. Let me know what you think, and if you agree in making this a separate PR, I'll create the issue for it.

Ok, then please create a new issue about updating the exercise generator files that describes the 2 reasons there are issues (the one with the changes signature and the other one with the new canonical data format you noticed earlier).

We should maybe wait with updating the exercise generators until we have split up the func Gen.

Here a script to get all exercises with a gen.go formatted as task (has to be started at root of project):

# ! /bin/bash

find ./exercises/ -name 'gen.go'|while read fname; do
  exercise=$(echo "$fname" | cut -d/ -f4)
  echo "- [ ] $exercise"
done

[eklatzer]

Two more things I forgot to mention:

• THANKS! 🙂
• Please create an issue about updating/extending the README regarding generators. With this new system there are some things to be aware of when writing a generator that should be pointed out (e.g. that cases are flattened, that the key is the property).

Done: #2300