exercism / go-test-runner

GNU Affero General Public License v3.0
15 stars 17 forks source link

Allow to enable race detector for certain exercises #62

Closed junedev closed 2 years ago

junedev commented 2 years ago

Currently, we don't use the race detector in the test runner. As @bitfield pointed out here https://github.com/exercism/go/issues/2021 it would be beneficial for some exercise to run it.

Here some initial thoughts the topic:

andrerfcsantos commented 2 years ago

I don't remember specifically checking if the race detector works on the alpine image, we should double check that.

I agree with a key in configlet. My suggestion would be to have a key containing a list of additional flags to the test command. That way we can not only specify if the race detector should be enabled, but other things, like fuzz tests if we want to do them in the future.

mhmmdd commented 2 years ago

I tested Golang -race parameter in golang:1.17-alpine3.14 docker image adding RUN apk add --update build-base make and it looked good. But that additional command makes the old image 198mb larger. If it is OK for you I can send a PR. @junedev

junedev commented 2 years ago

Thanks for investigating!

If I understand correctly, it does not work without the additional command.

If that is the case it would be great if you could check whether it works in golang:1.17-buster without an additional install command. (The test runner was using buster up until recently so in general there shouldn't be an issue with that.)

If buster works than it is preferable imo to download the bigger image (buster is also +200mb vs alpine) instead of downloading the smaller one and waiting for the install command to run, e.g. when building locally. The download is usually much faster.

mhmmdd commented 2 years ago

I don't know why you want to choose buster image even though it's bigger. Does it provide extra functionality? Yes, alpine image doesn't run -race command correctly without additional command. When I built the image I saw that -race parameter in the buster image worked fine.

Image Size
golang:1.17-buster 884MB
golang:1.17-alpine3.14 315MB
golang:1.17-alpine3.14
with additional command
(RUN apk add --update build-base)
513MB
junedev commented 2 years ago

@mhmmdd Just looking at the compressed sizes in Dockerhub, I thought alpine + extra command might be about the same size than buster. In that case, buster would have been better because we don't have to spend time on the extra command when building the image.

As you showed, my assumption was wrong so nevermind. Sorry for the confusion.

As for the PR, it doesn't make sense to just increase the image size as long as we don't use the race detector. A PR should include processing the exercise config. Not sure whether we already have documentation about the configlet version. I need to check. Would you be interested on working on that other part as well (once we clarified what the config looks like)?

junedev commented 2 years ago

I found the specification for the new field in the config file (https://github.com/exercism/configlet/#configlet-fmt), it is called custom. That means the file .meta/config.json could look like this:

{
   // ... some other stuff here ...
   "custom": {
      "testingFlags": ["-race"]
   }
}

@andrerfcsantos Can we agree on the above format or do you have a better idea?

As @andrerfcsantos mentioned, we want to allow more/other flags in the future. Would be nice if the code could already account for that.

If there is no custom key, no testingFlags field or the array is empty, no additional flags should be applied, the test runner should just work at is does right now.

andrerfcsantos commented 2 years ago

Sounds good!

junedev commented 2 years ago

@mhmmdd Since we know how the config file will look like know, would you like to work on reading that file and applying the config so it becomes available for the exercises?