Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New test runner design #130

Merged
merged 45 commits into from
Aug 15, 2024
Merged

New test runner design #130

merged 45 commits into from
Aug 15, 2024

Conversation

mk-mxp
Copy link
Contributor

@mk-mxp mk-mxp commented Aug 10, 2024

As suggested in #110 a re-implementation of all current use cases (test cases) using PHPUnit events. This reduces the complexity a lot.

@exercism/maintainers-admin I changed code ownership, as this replaces the existing test-runner completely.

@homersimpsons Do you want to take a look, also?

I also optimized the files remaining in the Docker image:

#23 [runtime 7/7] RUN ls -la && ls -la bin
#23 0.057 total 136
#23 0.057 drwxr-xr-x    5 root     root          4096 Aug 10 09:02 .
#23 0.057 drwxr-xr-x    1 root     root          4096 Aug 10 09:02 ..
#23 0.057 -rw-r--r--    1 root     root          4345 Aug 10 09:02 CODE_OF_CONDUCT.md
#23 0.057 -rw-r--r--    1 root     root         34522 Aug 10 09:02 LICENSE
#23 0.057 -rw-r--r--    1 root     root          1936 Aug 10 09:02 README.md
#23 0.057 drwxr-xr-x    2 root     root          4096 Aug 10 09:02 bin
#23 0.057 -rw-r--r--    1 root     root           262 Aug 10 09:02 composer.json
#23 0.057 -rw-r--r--    1 root     root         59562 Aug 10 09:02 composer.lock
#23 0.057 -rw-r--r--    1 root     root           372 Aug 10 09:02 phpunit.xml
#23 0.057 drwxr-xr-x    2 root     root          4096 Aug 10 09:02 src
#23 0.057 drwxr-xr-x   10 root     root          4096 Aug 10 09:02 vendor
#23 0.058 total 12
#23 0.058 drwxr-xr-x    2 root     root          4096 Aug 10 09:02 .
#23 0.058 drwxr-xr-x    5 root     root          4096 Aug 10 09:02 ..
#23 0.058 -rwxr-xr-x    1 root     root          2172 Aug 10 09:02 run.sh
#23 DONE 0.1s

mk-mxp added 30 commits August 4, 2024 17:06
- Have a Tracer class to track test runs
- Extract required data from Passed events
- Produce JSON from Result objects
- Inject the output file name by environment variable, use fallback
  name for local runs
This only changes the test names from method names to what PHPUnit sees
as prettified test name.
- Use PHPUnit from vendor/*
- Provide env var with result file path
- Strip out no longer required log files and post-processor

The Dockerfile must be written further. composer dependencies should be
installed in build step and only "vendor/*" transfered to the runtime
image.

bin/run.sh still contains debug output.
We don't have to disallow output on the command line anymore, the event
fires always.
Also must fail total result on one failed test in list
This must have the absolute exercise path, but cannot get it from
PHPUnit. So a new env var provides this.
@mk-mxp mk-mxp changed the title Draft: New test runner design New test runner design Aug 10, 2024
@mk-mxp mk-mxp self-assigned this Aug 10, 2024
@mk-mxp mk-mxp added x:action/improve Improve existing functionality/content x:knowledge/advanced Comprehensive Exercism knowledge required x:module/test-runner Work on Test Runners x:type/coding Write code that is not student-facing content (e.g. test-runners, generators, but not exercises) x:size/large Large amount of work x:rep/large Large amount of reputation labels Aug 10, 2024
Copy link

@homersimpsons homersimpsons left a comment

Choose a reason for hiding this comment

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

Thank you very much for this implementation! I find it way less hacky than the previous one. It also removes a lot of code which means it will be easier to maintain.

Thank you for improving the Dockerfile too. This would probably improve the execution time of the test runner.

.dockerignore Outdated Show resolved Hide resolved
.gitignore Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
src/Result.php Show resolved Hide resolved
src/Tracer.php Show resolved Hide resolved
src/Tracer.php Outdated Show resolved Hide resolved
src/Tracer.php Show resolved Hide resolved
src/Tracer.php Outdated Show resolved Hide resolved
@neenjaw neenjaw removed their request for review August 10, 2024 18:37
@mk-mxp mk-mxp requested a review from homersimpsons August 11, 2024 09:52
Copy link

@homersimpsons homersimpsons left a comment

Choose a reason for hiding this comment

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

Thanks for all those improvements!

Note: I let some threads open, but they are hidden, I think it would be great to make those changes, but if you don't want to feel free to skip those.

Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

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

LGTM I'll let the PHP-specific review for the other reviewers

@mk-mxp
Copy link
Contributor Author

mk-mxp commented Aug 13, 2024

Thanks to everyone! I'll merge on Thursday, as I have more time that day in case of unexpected things happening.

@mk-mxp mk-mxp merged commit ddd15b3 into exercism:main Aug 15, 2024
1 check passed
@mk-mxp mk-mxp deleted the new-test-runner-design branch August 15, 2024 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
x:action/improve Improve existing functionality/content x:knowledge/advanced Comprehensive Exercism knowledge required x:module/test-runner Work on Test Runners x:rep/large Large amount of reputation x:size/large Large amount of work x:type/coding Write code that is not student-facing content (e.g. test-runners, generators, but not exercises)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants