support workdir in matcher.FileMatcher{}

[dylanhitt]

Fixes #153

Hey @SimonBaeumer,

I found a bit of what I consider a bug today with file assertion. Currently we aren't respecting the working directory for opening files. I opened this quick fix, for right now. If you could take a look that would be great.

I'm currently working on adding a --workdir flag so workdir can be set from the command line as well. This is a little bit more involved than I thought. I plan to rewrite runtime.Validate to get the Matcher a bit easier, rather than having pass test.Command.Dir through multiple functions. I also plan on manipulating test_command.go, to make passing command line args a bit easier.

I'll probably be opening the branch off of this one, since I don't quite know my timeline on what I mentioned above. I mainly just wanted to get this fix in.

Cheers

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?

[SimonBaeumer]

Hey @dylanhitt!
I think this is expected behaviour. Since commander is started and we can assume that the files being read from are relative to commanders working dir.
Could you maybe give some more context on this if I missed something?

The --working-dir option looks like a good option, like make -C /dir.
Changing directory could be implemented with the syscall:
https://man7.org/linux/man-pages/man2/chdir.2.html
https://golang.org/src/os/file.go?s=8392:8420#L278

[dylanhitt]

Hey @SimonBaeumer

You're right that it is expected behaviour IF we are net setting the workdir in a tests config see. An example:

Imagine we are at the root of the project and I run the command commander test examples/commander.yaml. This test will currently fail.

it should match file output:
    command: printf "line one\nline two"
    config:
      dir: examples/
    stdout:
      file: _fixtures/output.txt

As the config.dir is not respected when asserting against a file. If I was to run something like:

it should match file output:
    command: printf "line one\nline two"
    config:
      dir: examples/
    stdout:
      file: _fixtures/output.txt

test2:
    command: echo "hello" > testfile.txt
    config:
      dir: examples/
    stdout:
      file: _fixtures/output.txt

the first test will still fail, while the second would create a file in examples/ called testfile.txt. I hope this clarifies where I'm coming from?

I looked into just using os.chdir to just change the workdir however, this could lead to possible race conditions if we ever attempt to introduce some form of concurrency this could lead to some tough problems to debug, if you don't think it's a problem we can take that route.

[SimonBaeumer]

Ahh okay!
I think it is okay to use the relative path from the execution directory for file assertions because they serve different purposes.
The dir inside the process should be set for the process which is spawned by commander for the test because:

• this also effects docker containers and tests executed viá ssh.
• execute one command in a /tmp directory leads to a file assertion like /tmp/../home/simon/.../result.txt

It is a good point regarding the race condition of chdir. My idea was more like changing the working dir only on commanders startup, so it is only called once by the main goroutine - all child routines inherent the new working dir.

This solve your issue with file assertions in the given use case. WDYT?

[dylanhitt]

Hmm,

Alright, and I'm fine with chdir at the beginning of execution. The only other thought I have on this is when reading the initial path when starting the execution and say we are at the root of the project i.e commander $ commander test --workdir examples/ examples/commander.yaml. This would ultimately fail, as we want to os.chdir before any of execution starts?

[SimonBaeumer]

If the working dir is changed I would assume that everything is relative to the --workingdir argument.
Imho the main point for this is to guarantee/enforce a consistent behaviour.
If it should not be relative to the working dir a full path is required i.e. $(pwd)/examples/commander.yaml.

[dylanhitt]

Got it. Let's close this, and I'll open a PR tonight for --workdir. I may refactor test_command.go a bit.

What do you think about supporting a struct for TestCommand. Something along the lines of:

type TestCommand struct {
  testPath string
  out output.OutputWriter
  ctx TestCommandContext
  // some other things
}

This would allows for a func (t *TestCommand) Setup(path, context). This could allow for easier configuration, especially for issues like #131 and #136. It's kinda funky passing options and test paths around to every function.

[SimonBaeumer]

I am looking forward for your PR!
Could you provide a little example PR for the struct? Not necessary that it compiles, it sound interesting to me 👍

btw travis looks like it has some problems with their job backlog ...