Skip to content

Conversation

@jiegillet
Copy link
Contributor

I've added a bin/run-tests-in-docker.sh in the CI that runs the analyzer on the exercises in test_data in docker.

I adapted bin/smoke_test.sh and bin/check_files.sh from elixir-test-runner.
They work by running the analyzer in every folder in test_data and comparing the result to expected results (that I added as well).

@jiegillet jiegillet added x:module/analyzer Work on Analyzers x:type/ci Work on Continuous Integration (e.g. GitHub Actions workflows) x:size/medium Medium amount of work labels Mar 5, 2022
Comment on lines 73 to 74
- name: Run Tests in Docker
run: bin/run-tests-in-docker.sh
Copy link
Member

Choose a reason for hiding this comment

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

Should both the step name and the bin filename mention that those are smoke tests only, not all tests?

# Example:
# ./bin/run-tests-in-docker.sh

set -e # Make script exit when a command fail.
Copy link
Member

Choose a reason for hiding this comment

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

👍 (sanity check: the script fails when either the first or the second stage of the docker build fails, which was the problem Erik wanted to address)

Comment on lines 8 to 17
# Temporarily disable -e mode
set +e
for solution in test_data/*/* ; do
slug=$(basename $(dirname $solution))
# run analysis
bin/run.sh $slug $solution $solution
# check result
bin/check_files.sh $solution
done
set -e
Copy link
Member

Choose a reason for hiding this comment

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

When I mess up with one of the expected results (e.g. change the emoji in test_data/lasagna/failing_solution/expected_analysis.json) to simulate a failure, running ./bin/run-tests-in-docker.sh still has an exit code 0. I think that means the CI won't really report smoke test failures.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, good catch.
I guess using set +e in this way is inappropriate.

@jiegillet
Copy link
Contributor Author

Sorry for the mess.
It turns out ALL the smoke tests were failing all along, and I didn't notice because

  1. The smoke tests were fine on my machine
  2. I used set +e which ignored the failures

The reason for failure was that the script didn't have permission to write the output file. I did however have the right to write to tmp so that's what I used.

I'm not too sure what I could have done for reason 1, I'm not too experienced with docker, but I thought the whole point was to have reproducible behavior...
Reason 2 however was a major mistake on my part, lesson learned.

@angelikatyborska
Copy link
Member

I think ideally the tests run in Docker would use the same docker run flags as in production, and the same as other tooling in their tests, for example:
https://github.com/exercism/c-test-runner/blob/main/bin/run-tests-in-docker.sh#L21-L24

There's no network, file-read only filesystem, but also mounting a temporary write filesystem to /tmp. If /tmp is the only directory that needs write permissions to make the smoke tests pass, that should work for us.

@jiegillet
Copy link
Contributor Author

According to Erik, they don't use read-only yet, but according to his next 7 words, they might in the future.
Yes, that config should work, I'll try it tomorrow!

Copy link
Member

@angelikatyborska angelikatyborska left a comment

Choose a reason for hiding this comment

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

Perfect! Now we will know in CI if something goes wrong with the Docker container.

@angelikatyborska angelikatyborska merged commit eb3ef2b into exercism:main Mar 6, 2022
@jiegillet jiegillet deleted the jie-test-in-docker branch March 6, 2022 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

x:module/analyzer Work on Analyzers x:size/medium Medium amount of work x:type/ci Work on Continuous Integration (e.g. GitHub Actions workflows)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants