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

Add test output check suggestion #123

Merged
merged 3 commits into from
Jan 5, 2018
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -61,11 +61,11 @@ Currently, you're interested in all of the `"cases"`. Each of these is a test c
Implement each of the test cases (along with any comments or explanations you think are necessary for people who aren't sure of themselves). When in doubt, check out the other, already implemented exercises for an example. Here's an example of implementing the test case shown above.

```bash
#!/bin/bash
#!/usr/bin/env bats

@test 'Say Hi!' {
run bash hello_world.sh

[ "$status" -eq 0 ]
[ "$output" = "Hello, World" ]
}
Expand All @@ -75,6 +75,8 @@ Implement each of the test cases (along with any comments or explanations you th

For each test you create, you should use the `description` value for the test case description. The `property` value is generally used by other languages to specify what the function to be run is called. Use your best judgement and check out how other exercises handle this. If there's just one property, you can just test the script as a whole. If there's multiple properties, consider testing those as either subcommands or flags (e.g. an exercise with an encode and a decode property could be handled by `run bash cyper.sh encode <argument>` or by `run bash cypher.sh -e <argument>`). You're the one writing the tests, so you decide. You'll get feedback when you open your pull request anyways, so don't stress too much about it.

For test cases that are expected to return `true` or `false` make sure you are expecting an output value. It is better to return `"true"` or `true`, to the console instead of just relying on the exit status in bash (0 for success, 1-255 for error). This will be consistent with the existing exercises and prevent possible issues in the future.

You can check to see if your example script works by running `bats`.

```bash
Expand All @@ -83,7 +85,7 @@ $ bats hello_world_test.sh

### Implementing an Example Solution

If you've been following along so far, your tests should fail. Go ahead and implement your solution in the `<your-exercise>.sh` file. Make sure your file has `#!/bin/bash` at the top.
If you've been following along so far, your tests should fail. Go ahead and implement your solution in the `<your-exercise>.sh` file. Make sure your file has `#!/usr/bin/env bash` ([shebang][shebang]) at the top.

Keep running your test file against it until your tests all pass. This process should help ensure that both your tests *and* your example are ship shape!

Expand Down Expand Up @@ -176,10 +178,10 @@ It's possible to submit an incomplete solution so you can see how others have co

### Cleaning Up

20. If you've been practicing good version control throughout this process, you may have several commits. At this point, you're almost ready to submit your pull request, but you should rebase against the most recent upstream master branch.
If you've been practicing good version control throughout this process, you may have several commits. At this point, you're almost ready to submit your pull request, but you should rebase against the most recent upstream master branch.

```bash
# Assuming you've alread 'git added' and 'git commited'
# Assuming you've already 'git added' and 'git commited'
# If you don't already have the exercism/bash as your upstream remote:
$ git remote add upstream https://github.com/exercism/bash.git
# To get the most recent upstream version
Expand All @@ -188,7 +190,7 @@ $ git fetch upstream
$ git rebase upstream/master
```

21. Now you're ready to sync up with Github and open your pull request!
Now you're ready to sync up with Github and open your pull request!

```bash
$ git push --force-with-lease origin <your-branch-name>
Copy link
Member

Choose a reason for hiding this comment

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

It is not in the scope of this PR, but why suggest a force push at all... even with lease. This should be a workflow management decision on the user, perhaps.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, but for now I think we can leave it as it is since it's a suggestion? Or do you think it will cause harm, I'm not aware of what --force-with-lease does (I did google it for a quick understanding).

Copy link
Member

Choose a reason for hiding this comment

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

TL;DR: I think I wanted to have this flagged for discussion before I forgot about it. It should be changed, though not in this PR.


It is advising on workflow, and this change surely doesn't belong to this PR, but I think the suggestion to use either form of 'force' is one that out-lies the scope of the Bash track. If this is a suggestion that is applicable as a "This is how this Exercism does it" then it should be at a higher level.

The advice itself isn't bad advice, but, here, it is neglectfully incomplete, as it is not even close to a magic bullet solution, and can itself cause some problem.

Expand All @@ -197,3 +199,5 @@ $ git push --force-with-lease origin <your-branch-name>
![Create pull request](img/create-pr.png)

Good luck, happy scripting, and thanks for your help!

[shebang]: https://en.wikipedia.org/wiki/Shebang_(Unix)