Remove skips from all tests

[bkhl]

These skips are mostly an annoyance when using bats, since it doesn't
have any command line flag to ignore skipped tests.

The output of bats is not that verbose anyway, so generally just running
all the tests from the outset should not overwhelm the student too much.

---

Reviewer Resources:

Track Policies

[kotp]

In other languages, we do comment out the skips. The same way we comment out the bats ones as we are working through the exercises.

Some folks use sed if they want to remove them, others may use their editor, such as VIM to make the changes.

On the plus side, the tests are not ordered randomly, so they always run in order, so aren't hard to watch, the changes (especially if not colorized) are still easy to see.

[kotp]

This Pull Request, if approved, would be missing the changes required in the instructions for the exercises where it states:

After the first test(s) pass, continue by commenting out or removing the skip annotations prepending other tests.

This is included, I believe, in every exercise as delivered.

[bkhl]

Removed those, and becoming hesitant now.

Here's the number of lines of output for each test, if running the tests now with no modification (in track order):

4 hello-world
10 two-fer
16 error-handling
55 raindrops
25 hamming
19 acronym
25 armstrong-numbers
31 pangram
79 bob
34 scrabble-score
34 grains
16 reverse-string
28 difference-of-squares
25 leap
28 rna-transcription
37 anagram
19 collatz-conjecture
16 nucleotide-count
43 phone-number
52 triangle
34 word-count
16 gigasecond
43 luhn
43 atbash-cipher
58 roman-numerals
49 affine-cipher
37 isogram
16 diamond
52 isbn-verifier

Maybe the amount of output from raindrops there is overwhelming.

So either maybe let's remove the skips from 'scrabble-score' onwards, or just drop this whole idea?

[giner]

Does anybody make these tests pass one by one? In my experience it goes something like: 50% test passed > 75% passed > 90% > 100%

[bkhl]

When I personally do exercises as a student, I just remove the ignores right away, or on other tracks run the tests with some test runner flags to run all tests, so as a student I'd personally prefer if this ignore/skip stuff was removed on all tracks.

However, I suppose the purpose of this is to not overwhelm students that are completely new to TDD with a massive screen of "failures", and also to get them into the flow of addressing one test case at a time. I can see the value of this as well.

[giner]

It would be nice to get input from somebody who sees this as a problem.

[giner]

On Golang track the tests are not skipped btw.

[guygastineau]

@giner true, but the golang tests don't proceed past the first failure in my experience. Their setup is ideal for TDD.

[guygastineau]

@bkhl I agree that the longer tests could he overwhelming to new users. I use tall terminals, so it presents less of a challenge, but users with a standard term height of 25 will face additional challenges.

[bkhl]

Maybe let's put something like this in there for now, mainly for mentor
convenience:

[[ $BATS_RUN_SKIPPED == true  ]] || skip

Simultaneously, I could try making a pull request to bats instead, to add
command line flags to run skip-marked tests, and/or for stopping after the
first failed test.

[guygastineau]

@bkhl That is a clever idea. If that line is present I think it should be explained in the readme, or even in a comment at the head of the exercises (even if mostly for mentors' convenience).

We should also test it against myriad workarounds that people are currently using, and we should ensure that this doesn't create odd behaviors for existing users. If you are willing to do the leg work on this then I am willing to approve and merge with another maintainer's approval.

If you are willing to make PR's to bats, then I think that is the most ideal long game solution to this problem. If you get a PR going over there and it seems like it is well received, then I would suggest not merging this PR. That way we wouldn't have to revert on our end after the bats PR is merged.

If they wont accept the PR, or if it will take unnecessarily long to merge, then I think merging this PR is reasonable.

Thank you for your hard work!

[kotp]

I would also make changes to bin/validate_exercises to reflect the way that students/mentors should skip the tests as well.

Right now it does reflect the "sed" way of commenting the skips at once, simulating how we suggest students to skip them over time.

[guygastineau]

I have opened an issue on the bats repo about a noskip option. I quick glance at the source makes this look pretty easy to implement over there.

noskip issue

[guygastineau]

So this is now mergeable in my opinion, but if we can get a noskip flag in bats itself I still think that is preferable.

I appreciate your work on this @bkhl , and I don't want you to feel like it was all for naught.

My suggestion is that we first proceed with a PR to the bats repo. I will have some time to work on that today, or I am willing to let someone else take a stab (or work together with someone else on the PR).

I think we can get a functional PR to bats today, and then if their maintainers don't respond by tomorrow afternoon or so I am willing to merge this PR as a quick interim patch.

@bkhl I appreciate the ingenuity of your work on this, and I think this workaround could teach important lessons to students using it. I am simultaneously concerned that it could be confusing to newer students.

Out of respect for your efforts I would very much like your opinion on the plan I outlined above. If it is to your liking, then we can plan to proceed. If it is not, then please feel free to provide your opinions and counter strategies😀

[bkhl]

I'm hacking away at a PR to bats now. Actually already done and just dealing with man page and stuff. Happy to let the PR here linger a bit until we see the response to that.

However, personally I've been using bats from the Ubuntu repos, and if a lot of people install in that or similar ways, it may take quite a while for the change to reach them.

[guygastineau]

@bkhl excellent. As an Arch user I forget that I am a minority for using rolling release.

I think that this quick fix should be merged then.

@bkhl @kotp if you're all in agreement, then I will merge this.

[bkhl]

I went ahead and made sstephenson/bats#265 now (some Travis test is failing and needs looking at though).

However, as I said don't think we should wait around for that, so happy to see this merged.

[glennj]

I just switched over to a macbook at work. Homebrew has recipes for both bats (https://github.com/sstephenson/bats) as well as bats-core (https://github.com/bats-core/bats-core). It seems neither is particularly active: they both have dozens of open pull requests. I wonder if it's worth an exercism-specific fork?

[kotp]

I'm hacking away at a PR to bats now. Actually already done and just dealing with man page and stuff. Happy to let the PR here linger a bit until we see the response to that.

I am too, at least.

I am not sure that this or the sed equivalent is more or less confusing. Before the written instructions had a comment about commenting out skips. Now we have a longer line to comment out. And the bin/valid_tests.sh file should still work, but it works on the premise that the skips were commented, regardless of the environment variable set for that command. This leads to a bit of confusion, like should I use the environment variable or should I comment the skips? What if I comment the skips, or some of them, and decide to use the environment variable, then should I edit the skip comments to be uncommented? Teaching a little bit of sed (and storing the command as a convenient tool) may be a solution that supports the bash ideology as well.

I like the idea of being able to set the environment variable to disable them all, but again, using sed to comment them out... once the skip is commented, I don't have to remember to comment them again or set that environment variable.

[guygastineau]

Haha, I didn't see that you are tine with the PR lingering @bkhl

I'll go ahead and merge #280 using regular skips.

Anyone want to take a quick look at it first?

[guygastineau]

@bkhl I just realized that once this is resolved (hopefully on bats' end) we will be able to substantially simplify our script for testing if example exercises pass the tests. So, thank you very much for your initiative on this.

[kotp]

Looks good, waiting on @bkhl to merge when ready!

[guygastineau]

Now we probably need to add all of the other exercises that have been ported by glennj, before this PR can be merged, or our track will be inconsistent.

[glennj]

I've got 16 exercises in my queue. If you folks want to prioritize this PR, I'll rip the skips out on my end.

[kotp]

I think the removals can be done in this PR... no need to coordinate. (It's programmatic to apply)... we would do a rebase on master and do the needful, right?

[bkhl]

Applied the same change to the exercises added since the last commit.

[guygastineau]

So are we wanting to go ahead and merge this?

[bkhl]

Looking back I'm also confused by the trail of conversation here. Just interpreted the last comment from kotp as my original merge request needing to be brought up to date with the added exercises.

[guygastineau]

Well, I for one, am happy to see this go through. @bkhl you said before you are happy to let this sit, but I it doesn't look like bats is going respond to your PR. Therefore, I leave it up to you. Whenever you want this merged just give the signal.

Side note for the future:

I know there had been mention of perhaps forking the bats project, so it would actually be maintained. I am of the opinion that there are multiple deficiencies that cannot be overcome in bats due to the fact that it is written in bash.

I love bash, but it is not a fully fledged programming language. Bats is sluggish, and I think performance benchmarking would be unreliable if added.

I have started work on a new shell testing program written in rust. Currently it can test any shell that supports the -c option. I am adding benchmarking and I'm working with a (still experimental) feature to load a shell lib in an ongoing process to test lib functions without the overhead of starting a shell each time. This last feature presents many more challenges, but would allow for proper benchmarking of library testing.

I am not quite ready to put this project on GitHub, but I think I am a couple weeks out.
The tests are json formatted, and I am half way through the creation of a new test generator that uses a .so written in rust for each exercise to generate the proper json for the tests.

As soon as I push it to GitHub I will let everyone here know about it. I would really appreciate all of your scrutiny and testing. Hopefully, the project will be good enough to supplant bats, and then we will have a maintained project for our resting software.

Finally

Thank you all for indulging me. I am excited about the project, but I imagine (if it is successful and accepted) that it will still be until late this year or sometime next year before it is adopted by the bash track.

Therefore, I reiterate my support of moving forward with this PR.

Thank you for all of your work @bkhl

[kotp]

It appears that as of March 18th, the repository to raise issues and commits on would be bats-core/bats-core per sstephenson/bats#150.

[guygastineau]

Thanks for the heads up @kotp

[guygastineau]

Perhaps @bkhl should submit the PR to that repo then.

bats will remain fine for our use case if they keep going (it does look like they are looking for more maintainers again).

Regardless I will let you all know when I release my project in case anyone wants to check it out, or hack a little rust in a PR😀

[kotp]

Looking back I'm also confused by the trail of conversation here. Just interpreted the last comment from kotp as my original merge request needing to be brought up to date with the added exercises.

I was just stating that if the exercises keep coming in from @glennj collection of 16 exercises before this is merged in, we should be able to facilitate the changes needed to bring them up to date.

[glennj]

I would suggest we should merge this in first. I can update my code before submitting for PR.