Build extra Go packages into the docker image

[andrerfcsantos]

Similar to what other tracks already have, this builds extra Go packages into the docker image so students can use them in their solutions.

The list of packages was discussed here exercism/go#2379

[coveralls]

Pull Request Test Coverage Report for Build 3214373255

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 77.445%

---

Totals
Change from base Build 3184568547:	0.0%
Covered Lines:	388
Relevant Lines:	501

---

💛 - Coveralls

[junedev]

@andrerfcsantos Please don't merge yet. I have some input but didn't have the time to test it out yet.

[junedev]

Here is my counter-proposal on how to manage this:
487a310

I think this is a cleaner way. Let me know what you think.

The draft PR here #85 also contains an example you can run with the docker command from the readme, just with external_dependency instead of gigasecond.

[andrerfcsantos]

While considering alternatives, I tried to optimize for easy instructions, both for users and for maintainers.

I've also thought of making the external dependencies a dependency of the test runner, but ultimately decided against it because:

• The test runner might have its own dependencies in the future, and I don't like mixing them with the list of external dependencies we support. As I see it, this causes 2 problems:
  1. The root go.mod file can't be used as a list of dependencies we support, because it might be mixed with dependencies the test runner itself uses. One would have to check external_dependencies/deps.go to see the dependencies they could use, but refer to the root go.mod file to check the version. This means that for each dependency, there are 2 files we now must sure are in sync and we must refer people to them to see the external dependencies we support. I'd prefer to have the list of dependencies just in one file that we can refer to everyone. If to overcome this we want to provide a 3rd file with a nice list of dependencies, that is now 3 files we must keep in sync.
  2. If the test runner wants to use a dependency for itself that we also want to support externally, it would have to be the same version of the dependency, as they are managed by the same go.mod file. I see this as an arbitrary limitation that a separate module controlling the external dependencies fixes.
• The point of dependencies.txt is to be a nice list that we can show our users with a list of dependencies supported and their versions, with the added benefit of each line being go get-able. The goal here was to make the instructions for students be as easy as "this is a list of dependencies and their versions we support, if you want to use it, go get it and upload the go.mod file along with your solution". To goal was also to make life easier for maintainers. Since this is also the file maintainers can use to control the dependencies, it means maintainers also only have to manage this file to control the dependencies. No other files to keep in sync, no extra commands to run.

Some other considerations:

• Ultimately, I also don't really like having a separate module to control the external dependencies. Ideally, something like GO111MODULE=off go get <external_dependency> outside any module (or even inside) could put the dependency in GOPATH forever, and that would be the end of the story, but I couldn't make it work. In the end, I think the benefit of separating the dependencies of the test runner from the ones we want to support outweighs the slight annoyance that is having an extra module.
• Also in the spirit of making the instructions as simple as possible for maintainers, I intentionally made the script generate the go.mod and go.sum of this extra module, instead of making them files you have to commit to manage the dependencies and then use go download. I figured the go.mod and go.sum of this extra module might only be useful for debugging, so I didn't want to make creating these files to manage the list of dependencies something mandatory.
• I do like the tests in your PR, and we can add them here. It is something I wasn't sure how to do or even if it was needed for this particular case, but I like the way you did it in the PR.

[junedev]

Thanks for clarifying your reasoning here.

While I understand the theoretical arguments, I don't think there is a very strong case.

• In Go, you usually should not rely on some very specific version of a dependency being used because of the minimal version selection algorithm. And the major version is visible from the file with the imports.
• I think only very few people will ever make use of the external dependencies and even fewer people will care about checking for exact versions of a package so a simple "good enough" solution would suffice.
• If you want to have a separate go.mod file in my version, we could make testrunner one module and the external deps another one and then we have a separate go.mod file with only the extra dependencies. I think telling people to add "@" is ok.
• In theory, manual version mgmt also has disadvantages. In your version, when package A depends on B but B is also in the list we would overwrite the version of B that A needs with whatever is hardcoded in the file and A might not work properly anymore. That is why dependency mgmt. exists in the first place.

Anyway, you can merge this if you want.

Re the "test" I added: It is not an actual test case for the test runner. It is really only an example that can be executed manually. I think CI does not execute the tests inside the final docker container so a normal test case executed via CI would not tell us whether the external dependency mgmt works.
(@ErikSchierboom mentioned some kind of golden/end2end tests but I don't know how that was set up.)

[andrerfcsantos]

Thanks for explaining.

There are good points there.

I'll defer merging, maybe we should think about this a bit more.

[ErikSchierboom]

@junedev I don't think the golden tests are setup here. I would have expected a bin/run-tests-in-docker.sh file here: https://github.com/exercism/go-test-runner/tree/main/bin Adding golden tests should be fairly straightforward though:

1. Copy the run-tests.sh and run-tests-in-docker.sh files from https://github.com/exercism/generic-test-runner/tree/main/bin
2. Add a tests directory that contains sub-directories for the different types of scenarios you want to test. The way this works is to have these sub-directories be like regular exercises, but setup in a way to have a specific type of output. This output is then included in the sub-directory in the form of a expected_results.json file.
   I'd include the scenarios listed in https://github.com/exercism/generic-test-runner/tree/main/tests, but you'd also add an external packages example.
3. Run the ./bin/run-tests-in-docker.sh file within your test.yml workflow: https://github.com/exercism/go-test-runner/blob/main/.github/workflows/test.yml (see https://github.com/exercism/generic-test-runner/blob/main/.github/workflows/ci.yml#L20)

[andrerfcsantos]

@junedev Updated the PR to use something similar to your version. The difference is that I kept the separate module for dependency management, as I think is cleaner this way. Let me know if this fully addresses your concerns or if there is something I overlooked. I think this is better indeed, so thanks for the feedback 👍

@ErikSchierboom Thanks for the summary!

@ErikSchierboom @junedev If that's ok by you, I'd be more comfortable with addressing the golden tests in a separate issue/PR. I added tests in this PR, but like june said, these tests are not running in CI anywhere as far as I can tell (I did run them locally using docker with no network access and they are passing). Maybe we can address running tests in CI together with the golden tests as they seem to be closely related tasks.

[junedev]

Thanks for the update! Two more comments around this:

• I wonder about the term "extra packages". I have not really heard that before. Shouldn't it say "external" or "third-party" instead of "extra"?
• You added 2 examples/tests now. Are they covering different aspects? If not, one would be enough.

[andrerfcsantos]

I wonder about the term "extra packages". I have not really heard that before. Shouldn't it say "external" or "third-party" instead of "extra"?

External packages sound good, changed the naming to it.

You added 2 examples/tests now. Are they covering different aspects? If not, one would be enough.

The new test added uses code that imports golang.org/x/exp/contraints, so the idea here is to test if golang.org/x/exp is being built in the image. The idea is to have 1 test (at least) per external dependency supported.

[junedev]

One more thing I realized about this new version: We now declare a module within a module. While this works technically, I vaguely remember it was not recommended in some Go guidelines.

I am not aware of any practical disadvantages, so feel free to merge as is. The alternative would be to move the root level go.mod etc into the testrunner folder but then go test ./... in the root folder would not work anymore even with a workspace file so there would be disadvantages to this approach.