Closed andrerfcsantos closed 1 year ago
Totals | |
---|---|
Change from base Build 3184568547: | 0.0% |
Covered Lines: | 388 |
Relevant Lines: | 501 |
@andrerfcsantos Please don't merge yet. I have some input but didn't have the time to test it out yet.
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:
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.go.mod
file. I see this as an arbitrary limitation that a separate module controlling the external dependencies fixes.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:
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.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.Thanks for clarifying your reasoning here.
While I understand the theoretical arguments, I don't think there is a very strong case.
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.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.)
Thanks for explaining.
There are good points there.
I'll defer merging, maybe we should think about this a bit more.
@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:
run-tests.sh
and run-tests-in-docker.sh
files from https://github.com/exercism/generic-test-runner/tree/main/bintests
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../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) @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.
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.
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 https://github.com/exercism/go/issues/2379