-
-
Notifications
You must be signed in to change notification settings - Fork 361
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
Add test
command to run any unit test
#1092
Conversation
Hello. Thanks for opening a PR on Exercism. We are currently in a phase of our journey where we have paused community contributions to allow us to take a breather and redesign our community model. You can learn more in this blog post. As such, all issues and PRs in this repository are being automatically closed. That doesn't mean we're not interested in your ideas, or that if you're stuck on something we don't want to help. The best place to discuss things is with our community on the Exercism Community Forum. You can use this link to copy this into a new topic there. Note: If this PR has been pre-approved, please link back to this PR on the forum thread and a maintainer or staff member will reopen it. |
cmd/test.go
Outdated
autoSeparateArgs bool | ||
} | ||
|
||
var testConfigurations = map[string]TestConfiguration{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might make more sense to read this data from a user-configurable place: a user might want to modify the standard command, or add a missing one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I can look into that.
What configuration do you expect users to do? Replace the command entirely, always add specific arguments, etc. Off the top of my head, I'm thinking we add a field to config.json
that covers cases we want to start with (testCommand
, args
, etc). Or do you mean something central? They could maybe override commands (and args, etc) per-track in a central place.
I think it would make sense for it to be user-overridable, but to include these defaults in the CLI. That way the command will work out of the box for most people without having to modify the config file, but the option is there if people want it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's a thought: use ~/.config/exercism/user.json
Extend exercism configure
exercism configure --test --track go --script "go test"
exercism configure --test --track ruby --script "(cat <<'END'
sh -c '
for t in *_test.rb; do
gawk -i inplace '1; /< Minitest::Test/ {print " def skip; end"}' "$t"
ruby "$t"
done
'
END
)"
And then ~/.config/exercism/user.json contains
{
"apibaseurl": "https://api.exercism.io/v1",
"token": "my-uuid",
"workspace": "/Users/glennj/src/exercism/exercism.io"
"testing": {
"go": "go test",
"ruby": " sh -c '\n for t in *_test.rb; do\n gawk -i inplace '1; /< Minitest::Test/ {print \" def skip; end\"}' \"$t\"\n ruby \"$t\"\n done\n '"
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not opposed to making it so configurable, but I wonder if we shouldn't release a simpler version initially and then add configuration options later?
With the universal test runner, one of my main design goals was "it just works". I think being able to run every test suite out of the box without configuration is a great starting point and will cover a lot of use cases and we can iterate from there.
cmd/test.go
Outdated
testConf, ok := workspace.TestConfigurations[track] | ||
|
||
if !ok { | ||
return fmt.Errorf("test handler for the `%s` track not yet implemented. Please see HELP.md for testing instructions", track) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice to point to the HELP.md
file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also noticed that every track's testing instructions are at https://exercism.org/docs/tracks/{TRACK}/tests
, so I could link out to that too (or instead).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could, but I think HELP.md
is even better in this case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should maybe no use "test handler", which is somewhat technical, but use a more functional description. Maybe:
return fmt.Errorf("test handler for the `%s` track not yet implemented. Please see HELP.md for testing instructions", track) | |
return fmt.Errorf("This track does not yet support running tests via the CLI. Please see HELP.md for testing instructions", track) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, great call. I went with the \"%s\" track does not yet support running tests using the Exercism CLI. Please see HELP.md for testing instructions
, to be clear tests can be run via a CLI, just not the exercism one. Also I kept the track name in, since that reminds them where they are (to reduce confusion vs "this track")
} | ||
|
||
// NewExerciseMetadata reads exercise metadata from a file in the given directory. | ||
func NewExerciseConfig(dir string) (*ExerciseConfig, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like how this is now "a thing". Maybe the getExerciseSolutionFiles
function in workspace/submit.go
can also be updated once this PR is merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah! that would make sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I think that covers the feedback we had, and testing seems promising.
The thing I'm not confident about is the best way to test it. In my python version, I focused mostly on two cases:
- given a test configuration, does it match a specific directory setup
- given a specific set of files, what's the resulting command when the top-level command is run
This is a little different, since we don't have a matching system. As long as reading the config works across tracks, this will just work. I think we'd want to:
- verify that test file replacement is happening
- verify that windows commands are being prioritized if available
- verify we can read configs (and error handling happens correctly)
I haven't done much unit testing in go, so I'd love to defer to someone else on these if it's something you (or another contributor) has a lot of experience with. I was playing with mocking the syscalls and directory lookups, but wasn't having much luck; there may be a better way to go about it.
// MockInteractiveResponse: "first-input\nsecond\n", | ||
// } | ||
// | ||
// cmdTest := &CommandTest{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI was complaining about this block, so I ran go fmt ./...
and this was the change. I realize you want to keep the PR focused, but I also assume you want to keep the CI green
WindowsCommand: "test.ps1", | ||
}, | ||
"coffeescript": { | ||
Command: "jasmine-node --coffee {{test_files}}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I experimented with putting {{test_files}}
in a const (which would improve the replacement code), but it made writing the commands less clear (one of "ruby " + TEST_FiLES
or fmt.Sprintf("ruby %s", TEST_FILES)
or injectTestFilesPlaceholder("ruby %s")
, all of which seemed to defeat the point).
As written, there's a higher chance that someone adding a new command will typo the placeholder and the replacement won't happen, but it provides the most clarity when reading the actual commands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is nice!
I'd like someone with Go experience to look at this (@ekingery or @junedev perhaps?). I'm doing some functional testing:
|
workspace/test_configurations.go
Outdated
Command: "test.sh", | ||
WindowsCommand: "test.ps1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Command: "test.sh", | |
WindowsCommand: "test.ps1", | |
Command: "./test.sh", | |
WindowsCommand: "pwsh test.ps1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated, but was just copying from the docs. they may need to be updated: https://exercism.org/docs/tracks/cobol/tests#h-windows
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ErikSchierboom The Go code itself looks fine to me. Can't comment on general code organization in the codebase or anything like that though.
Sidenote: On a conceptual level, I am not sure whether I am a big fan of this feature as we take away learning about how to actually run tests in the language. Even if it means just learning that in this language usually a make file is used or "there is a built-in test runner" like in the case of Go. I am ok with this being merged but I wanted to mention it.
Ok, I've added little unit tests for the exercise config reading. It doesn't look like it's easy to test the windows behavior in a unit test (based on this SO answer), but I have a windows computer I can probably install
That's a great point! I got similar feedback on the standalone project. I've added a logline that shows exactly what's being run, so at least it's more clear what's happening:
If we're comfortable (enough to move forward) with the level of unit tests written, I think the last big tasks is go through the rest of the "easy" and "medium" language tracks and add them into the test configurations. I've been saving that for last so the PR doesn't get too unwieldy, but can grab it tomorrow. |
Ok, I've added more tests than I thought I'd be able to. I saw that CI actually has a windows runner, so I added a test that pulls the windows command. I also got some file-based tests working by actually writing and removing files. It's in the test directory, which I don't love, but it should be fine. The last thing is the actual Thanks for bearing with me y'all! |
@ekingery Are you up for a code review? I'm especially interested in the test coverage. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution, @xavdid! I'm rusty now in both Go and this CLI codebase, however everything looks in line with what I'd expect and the test coverage is solid. Might be nice to add coveralls to this project for an empirical assessment on test coverage. All that said, 👍 from me.
Thanks for the reviews everybody! |
Really happy about the state of this PR! There are two things left to do:
|
Ah, pesky platform-specific error message. I think I fixed CI in 540bef5- if you could give that another run, I'd appreciate it! I'll get the rest of the test commands in tomorrow. |
Ok, I've added the remaining tracks (and more than I thought I'd get)! Work is split into a few last commits:
@ErikSchierboom unless anything from those jumps out at you, I think we're all set! The only remaining tracks are |
I'm quite busy today, so it will be next week (wednesday at the earliest). Sorry! |
Ok, I debugged the tests on my windows machine and got them working in 238b572. Turns out windows won't delete a folder if it's got an open descriptor somewhere and I had neglected to close after write. Mac had no issue with this, so it was news to me. Also, I was using That CI should be good to go. 🤞 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works like a charm! Just a couple of minor comments (and CI is failing)
cmd/test.go
Outdated
testConf, ok := workspace.TestConfigurations[track] | ||
|
||
if !ok { | ||
return fmt.Errorf("test handler for the `%s` track not yet implemented. Please see HELP.md for testing instructions", track) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should maybe no use "test handler", which is somewhat technical, but use a more functional description. Maybe:
return fmt.Errorf("test handler for the `%s` track not yet implemented. Please see HELP.md for testing instructions", track) | |
return fmt.Errorf("This track does not yet support running tests via the CLI. Please see HELP.md for testing instructions", track) |
cmd/test.go
Outdated
cmdParts = append(cmdParts, args...) | ||
} | ||
|
||
fmt.Printf("--> %s\n", strings.Join(cmdParts, " ")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like the logging! My only concern is that it might be too subtle for the student to notice. Maybe we could make it more explicit? To instead of --> <command>
, something like Running tests via: <command>
or Running test command: <command>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure! updated to:
Running tests via `<command>`\n\n
which makes it clear exactly what's running and gives it some space to breathe
Ok, addressed that feedback. CI should be good to go as well! |
return cmd, nil | ||
} | ||
|
||
var TestConfigurations = map[string]TestConfiguration{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just done a quick double check to see which active tracks are missing.
This is the list of missing tracks:
- abap
- common-lisp
- cpp
- delphi
- fortran
- objective-c
- pharo-smalltalk
- plsql
- powershell
- r
- scheme
- unison
- vimscript
Could you double check these tracks? If they're not supported, it might be useful to add a comment somewhere noting that those tracks are not supported (and why)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, added in ad33864! I've confirmed that most of the remaining tracks are N/A (tests are run in IDE, etc).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For common-lisp, I used
sbcl --load "${slug}-test" \
--eval "(exit :code (if (${slug}-test:run-tests) 0 5))"
That assumes a particular distribution: there are several suggested on the track's intstallation page.
For fortran, I have this fish snippet
if not test -d ./.exercism
echo run me from the exercise root >&2
return 1
end
mkdir -p build
cd build
test -d ./testlib; or cmake -G "Unix Makefiles" ..
make; and ctest -V
cd ..
R is just
Rscript test_*.R
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
R is just
Rscript test_*.R
Could you update the configuration to use this command? I can confirm that this is also how the test runner does it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good call! I didn't even think about the test runners - I've been going off the public docs
"vbnet": { | ||
Command: "dotnet test", | ||
}, | ||
// vimscript: tests are run from inside a vim session |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at how the test runner does it, it looks like this might be possible. Could you double-check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this does seem to work, the output is super long and ugly. There doesn't seem to be a --quiet
flag and I'm disinclined to ship it as is:
% vim -XN -i NONE -c ":so hello_world.vim" -c "Vader! hello_world.vader"
Vader note: cannot print to stderr reliably/directly. Please consider using Vim's -es/-Es option (mode=n).
VIM - Vi IMproved 9.0 (2022 Jun 28, compiled Apr 15 2023 04:26:46)
macOS version - arm64
Included patches: 1-1424
Compiled by [email protected]
Normal version without GUI. Features included (+) or not (-):
+acl -dnd +listcmds +postscript +termresponse
-arabic -ebcdic +localmap +printer +textobjects
+autocmd -emacs_tags -lua -profile +textprop
+autochdir +eval +menu -python +timers
-autoservername +ex_extra +mksession -python3 +title
-balloon_eval +extra_search +modify_fname +quickfix -toolbar
-balloon_eval_term -farsi +mouse +reltime +user_commands
-browse +file_in_path -mouseshape -rightleft -vartabs
++builtin_terms +find_in_path +mouse_dec -ruby +vertsplit
+byte_offset +float -mouse_gpm +scrollbind +vim9script
+channel +folding -mouse_jsbterm +signs +viminfo
+cindent -footer +mouse_netterm +smartindent +virtualedit
-clientserver +fork() +mouse_sgr -sodium +visual
+clipboard -gettext -mouse_sysmouse -sound +visualextra
+cmdline_compl -hangul_input +mouse_urxvt +spell +vreplace
+cmdline_hist +iconv +mouse_xterm +startuptime +wildignore
+cmdline_info +insert_expand +multi_byte +statusline +wildmenu
+comments +ipv6 +multi_lang -sun_workshop +windows
+conceal +job -mzscheme +syntax +writebackup
+cryptv +jumplist +netbeans_intg +tag_binary -X11
+cscope -keymap +num64 -tag_old_static -xfontset
+cursorbind +lambda +packages -tag_any_white -xim
+cursorshape -langmap +path_extra -tcl -xpm
+dialog_con +libcall -perl +termguicolors -xsmp
+diff +linebreak +persistent_undo +terminal -xterm_clipboard
+digraphs +lispindent +popupwin +terminfo -xterm_save
system vimrc file: "$VIM/vimrc"
user vimrc file: "$HOME/.vimrc"
2nd user vimrc file: "~/.vim/vimrc"
user exrc file: "$HOME/.exrc"
defaults file: "$VIMRUNTIME/defaults.vim"
fall-back for $VIM: "/usr/share/vim"
Compilation:
gcc -c -I. -Iproto -DHAVE_CONFIG_H -DMACOS_X_UNIX -g -O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=1
Linking: gcc -L/usr/local/lib -o vim -lm -lncurses -liconv -framework Cocoa
Starting Vader: 1 suite(s), 1 case(s)
Starting Vader: /Users/david/projects/exercism/vimscript/hello-world/hello_world.vader
(1/1) [EXECUTE] Say Hi!
Success/Total: 1/1
Success/Total: 1/1 (assertions: 1/1)
Elapsed time: 0.01 sec.
Do you think it's common that users are running this standalone vs inside vim itself?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not. The verbosity is indeed a bit much. Let's leave it out
return cmd, nil | ||
} | ||
|
||
var TestConfigurations = map[string]TestConfiguration{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
R is just
Rscript test_*.R
Could you update the configuration to use this command? I can confirm that this is also how the test runner does it.
workspace/test_configurations.go
Outdated
@@ -90,6 +92,8 @@ var TestConfigurations = map[string]TestConfiguration{ | |||
"coffeescript": { | |||
Command: "jasmine-node --coffee {{test_files}}", | |||
}, | |||
// common-lisp: tests are loaded into a "running Lisp implementation", not the CLI directly | |||
// cpp: tests are mostly run via IDE; only linux seems to support `make`. There's also a more complex test creation process (using `CMake`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we do cmake and then make via &&
? See https://github.com/exercism/cpp-test-runner/blob/main/bin/run.sh#L43 We'd have to test if this works in Windows of course.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It won't work as written. It works if I run cmake . && make
manually, but because we're passing the command into a single exec.Command
, it's being run as cmake . make
, and make
is ignored as an unknown path:
CMake Warning:
Ignoring extra path from command line:
"make"
To make that work, we'd need to make every command a slice of strings (most of which will only have 1 item) and then run each in order (which will work cross-platform).
I think that's a reasonable improvement to make, but I'd recommend putting it in its own PR; this one has gotten big. I also have no idea if/how this build script works on windows- I left off the -G "Unix Makefiles"
part of the command and I have no idea how important that is. Verifying this is probably best left to someone who knows more about those tracks. I'd rather ship a TODO than a solution we're not sure works
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, it looks like the cmake
portion is 1-time setup. Once it's run successfully, the only thing you need to run tests is make
. I think it's reasonable to treat the cmake
portion as user-required setup/installation and the actual test run as make
. I'll update accordingly.
testercism
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just appreciating this new feature and noticed this file was added. I’m assuming it wasn’t meant to be committed so I thought I’d point it out 🤓
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good call, removed 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It all looks really good! There is one final thing to do, and that is to remove the committed testercism
binary from the history. It was added in a9c1fb1 and removed in 2991cfe, but that still means it is in the Git repo.
What you'll need to do is to an interactive rebase and edit the commit in which it was added to make sure that commit doesn't add the file and remove the commit in which the file was removed (which is unneeded).
Are you familiar with interactive rebasing? I tried doing it for you, but I can't push to your branch (it's a setting that you need to enable somewhere).
"vbnet": { | ||
Command: "dotnet test", | ||
}, | ||
// vimscript: tests are run from inside a vim session |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not. The verbosity is indeed a bit much. Let's leave it out
erik@schierheim:~/exercism/cli$ git log --all --full-history -- testercism
erik@schierheim:~/exercism/cli$ Nice! That looks good. Once CI is green, I'll merge. Thank you for this great contribution! |
All right! Looks like we're all set. You're very welcome, glad we got it over the line. Thank you for your patience with the whole process - this one was a journey 😅 |
@xavdid Sorry that it took a bit long, but this is a big feature and one that many students will benefit from! 🎉 |
@kytrinyx Could we maybe do the release together, to make sure I have all the right permissions? |
@ErikSchierboom For sure! Let's see what we can do this afternoon. |
`test` command was supported since v3.2.0, see a8ffc31 (Add `test` command to run any unit test (\exercism#1092), 2023-07-28)
Per discussion in this forum post, I've added a
test
command to the exercism CLI. Much like the universal test runner it's inspired by, its goal is to be able to run the basic unit test suite for any exercism exercise.It does this by:
.exercism/metadata.json
to get the track name.exercism/config.json
to get test file name(s) and including themexercism test
exec.Command
to run and return info about the exit codeTypes of Tests
Based on my (very brief) sampling of exercism tracks, there are 2 main ways tests are run:
cargo test
,go test
,lien test
, etc)ruby lasagna_test.rb
)So, the
TestConfiguration
struct covers both of those cases (by including an option to append the test file(s) to the command).The other configuration option aids in arg passing. Some language tools (namely
cargo
) expect arguments to the test command to be after a double dash (--
). Unfortunately, so doesCobra,
thego
arg parser. So, I was running into the following trying to run rust tests with--include-ignored
:exercism test --include-ignored
:--include-ignored
is not a valid flag for the test commandexercism test -- --include-ignored
:--include-ignored
is not a valid flag forcargo
, put it behind a--
exercism test -- -- --include-ignored
: works as expectedThis isn't great UX though, so I added an option for languages to be able to ask that args to their test runner be put behind an implied
--
. This way, the rust command works when run as option 2 above (which is in line with most other languages).TODO