Skip test case

[dylanhitt]

closes #127

Checklist

• Added unit / integration tests for windows, macOS and Linux?
• Added a changelog entry in CHANGELOG.md?
• Updated the documentation (README.md, docs)?
• Does your change work on Linux, Windows and macOS?

[dylanhitt]

@SimonBaeumer, I plan on finishing this implementation tomorrow. I'll then send over my ideas for how skipped test should be displayed in the output. We can the hash out our thoughts there.

Hope all is well

[dylanhitt]

@SimonBaeumer the implementation is ready for review. I plan on moving the printSkipped too cli_template.go once we agree on a formatted output. Currently:

commander (skip-test-case) $ ./commander test examples/commander.yaml 
Starting test file examples/commander.yaml...

✗ [local] it should fail
- [skipped] it should skip
✓ [local] it should print hello world
✓ [local] it should validate exit code
✓ [local] echo hello

Results

✗ [local] 'it should fail', on property 'Stderr'

Expected

/bin/sh: invalid: command not found

to contain

/bin/sh: 1: command invalid: not found


Duration: 0.026s
Count: 5, Failed: 1, Skipped: 1
Test suite failed, use --verbose for more detailed output

Let me know what you think, I can add some integration tests and docs soon there after.

[SimonBaeumer]

✗ [local] it should fail
- [skipped] it should skip
✓ [local] it should print hello world
✓ [local] it should validate exit code
✓ [local] echo hello

Displaying a - for skipped is fine.
Writing the status of the test inside the node field is a little bit weird, it would break scripts which depend on the given structure result [node] name.
Further if new scripts are created you can not rely on the output structure because in special cases it just changes, i.e. you want to fetch all nodes, and suddenly one node is named skipped.

Adding debug log statements in https://github.com/commander-cli/commander/pull/144/files#diff-cdb01b657d4a9de87f1466a8e642945dR34 when --verbose should be added :)

[dylanhitt]

Do you have any suggestions on the output when we printSkipped during runtime? Or should we just not print skipped tests at runtime? Maybe

// default to local
- [filename.yaml] [local] test name, was skipped

// leave blank []
- [filename.yaml] [] test name, was skipped

// remove bracket
- [filename.yaml] test name, was skipped

I elected to put debug logs here. It guarantees the printSkipped output is printed right after the debug logs. If it's in runner.go the output is sometimes affect by another TestResult on the channel, this way the debug logs and the skipped result are next to each other when printing --verbose. Ex.

2020/08/04 19:45:58 title: 'it should fail'  Command:  invalid
2020/08/04 19:45:58 title: 'it should fail'  Directory:  
2020/08/04 19:45:58 title: 'it should fail'  Env:  []
2020/08/04 19:45:58 title: 'it should fail'  ExitCode:  127
2020/08/04 19:45:58 title: 'it should fail'  Stdout:  
2020/08/04 19:45:58 title: 'it should fail'  Stderr:  /bin/sh: invalid: command not found
2020/08/04 19:45:58 title: 'it should fail'  Stdout-Expected:  {[] map[]  0 [] map[] map[]}
2020/08/04 19:45:58 title: 'it should fail'  Stdout-Result:  true
2020/08/04 19:45:58 title: 'it should fail'  Stderr-Expected:  {[/bin/sh: 1: command invalid: not found] map[]  0 [] map[] map[]}
2020/08/04 19:45:58 title: 'it should fail'  Stderr-Result:  false
✗ [local] it should fail
2020/08/04 19:45:58 title: 'it should skip'  Was skipped
2020/08/04 19:45:58 title: 'it should skip'  Command:  echo hello
2020/08/04 19:45:58 title: 'it should skip'  Directory:  
2020/08/04 19:45:58 title: 'it should skip'  Env:  map[]
- [] it should skip

[SimonBaeumer]

I would move the skipped check at the beginning of the for-loop because than you see directly that skipped test cases are ignored.

	for _, tr := range results {
		r := convertTestResult(tr)
                if r.Skipped {
                     continue
                }

		if r.Error != nil {
			w.fprintf(w.au.Bold(w.au.Red(w.template.errors(r))))
			w.fprintf(r.Error.Error())
			continue
		}
		if !r.Success {
			w.fprintf(w.au.Bold(w.au.Red(w.template.failures(r))))
			w.fprintf(r.Diff)
		}
	}

[SimonBaeumer]

Little typo Was -> was

[SimonBaeumer]

Do you have any suggestions on the output when we printSkipped during runtime? Or should we just not print skipped tests at runtime? Maybe

// default to local
- [filename.yaml] [local] test name, was skipped

// leave blank []
- [filename.yaml] [] test name, was skipped

// remove bracket
- [filename.yaml] test name, was skipped

I would like to see the node on which the test was skipped, that we stick to the same output format all the time. The cli output can be seen like an API because the results are processed, i.e. commander test | awk '{...}'', if we touch the output format this would lead to breaking changes and unreliable output.

Further it is defined that nodes create a testing matrix, that means every test gets executed on every node and is seen as it's own test case internally.

If the skip check is moved inside the t.Nodes range it can simply obtain the node.

			for _, n := range t.Nodes {
				result := TestResult{}
				for i := 1; i <= t.Command.GetRetries(); i++ {
					if t.Skip {
						result = TestResult{TestCase: t, Skipped: true, Node: n}
						break
					}

I elected to put debug logs here. It guarantees the printSkipped output is printed right after the debug logs. If it's in runner.go the output is sometimes affect by another TestResult on the channel, this way the debug logs and the skipped result are next to each other when printing --verbose. Ex.

Imho this finde for the moment, I am thinking about how the logs will be printed in the future. It feels not good to add this amount of verbosity inside the source code.
I would do it with #119

[dylanhitt]

@SimonBaeumer great idea to skip for all nodes! This should be the final commit.

Cheers

[codeclimate]

Code Climate has analyzed commit 352ac31 and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	1

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 92.4% (0.2% change).

View more on Code Climate.

[SimonBaeumer]

LGTM