Add file assertion #147

glesica · 2020-10-15T01:04:36Z

Fixes #105

Happy to discuss and make changes!

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 · 2020-10-16T16:44:35Z

Thank you for your PR and your interest in commander we're always looking for more contributor! Expect some feedback this weekend 😄

Cheers

SimonBaeumer · 2020-10-18T11:59:59Z

Makefile


 integration-linux: build
 	$(info INFO: Starting build $@)
 	./integration/setup_unix.sh
-	commander test commander_unix.yaml
-	commander test commander_linux.yaml
+	./commander test commander_unix.yaml


This was by intention. With this line you would use the dev-build for integration testing. Normally inside the path should be a stable version of commander, especially in CI environments like travis.

Oh I see, very meta! I'm not using GOPATH so I'll just add a stable build to my personal bin/.

SimonBaeumer · 2020-10-18T12:07:47Z

pkg/suite/yaml_suite.go

@@ -101,6 +102,24 @@ func convertNodes(nodeConfs map[string]YAMLNodeConf) []runtime.Node {
 func convertYAMLSuiteConfToTestCases(conf YAMLSuiteConf, fileName string) []runtime.TestCase {
 	var tests []runtime.TestCase
 	for _, t := range conf.Tests {
+		stdout := t.Stdout.(runtime.ExpectedOut)


This should be moved outside of the YAML convertion for several reasons.

Adding a new file format would need to expand the files too, resulting in duplicate code (i.e. adding json)

Separation of concerns would break here because the reading of the file is considered logic of the matcher and not of the YAML convertion

I can understand the approach by just expanding the file and using a TextMatcher under the hood. I would suggest introducing a FileMatcher instead, which does the same but at a different place.
The FileMatcher reads the file and matches it with the TextMatcher. Further it would allow easier testing of the FileMatcher with unit tests.
So basically it does the same, but in the matcher pkg.

Gotcha, that makes sense. I wasn't sure where ya'll would want the file I/O to happen since it always makes testing harder.

dylanhitt

I'm pretty much in agreement with Simon, if you have implementation questions please ask!

dylanhitt · 2020-10-18T14:25:25Z

On another note, I was thinking about how this would affect the verbose output. With the current implementation the large blob of text from the file would be printed Stdout-Expected: {[(large blob)] map[] 0 [] map[] map[]}, with moving the implementation to matcher we should see Stdout-Expected: {[] map[] 0 [] map[] map[] /path/to/file.txt}

I personally think this output isn't optimal. Do we want to think about writing a string method to make this printing better (such as labeling)? We could always make a note and bundle it into #119 @SimonBaeumer thoughts?

We may also want to think about better diffing especially for file assertion. If we implement the FileMatcher just like TextMatcher our diff would be something like

✗ [local] 'it should print hello world', on property 'Stdout'
--- Got
+++ Expected
@@ -1 +1 @@
-Lorem ipsum dolor sit amet, cons nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua.
+Lorem ipsm dolor sit amet, cons nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua.

This can be tough to see differences in very large files. Do we want to do something where we show differences a bit better, like pinpointing where differences occur by line? I think better diffing is out of the scope for this PR so i'm fine with the current implementation. Maybe we make a note on #112.

SimonBaeumer · 2020-10-18T15:35:00Z

On another note, I was thinking about how this would affect the verbose output. With the current implementation the large blob of text from the file would be printed Stdout-Expected: {[(large blob)] map[] 0 [] map[] map[]}, with moving the implementation to matcher we should see Stdout-Expected: {[] map[] 0 [] map[] map[] /path/to/file.txt}

I personally think this output isn't optimal. Do we want to think about writing a string method to make this printing better (such as labeling)? We could always make a note and bundle it into #119 @SimonBaeumer thoughts?

Would log.Logf("%+v", expectedOut) do the trick?

This can be tough to see differences in very large files. Do we want to do something where we show differences a bit better, like pinpointing where differences occur by line? I think better diffing is out of the scope for this PR so i'm fine with the current implementation. Maybe we make a note on #112.

I am with you on this, I would prefer a view which displays a table where it can be compared directly.
Nevertheless if the difference is in the same line I think it is hard to underline the diff without writing a library which does the highlighting for us.

glesica · 2020-10-20T00:06:58Z

OK, this should be more in line with what ya'll were thinking. I omitted a unit test in validator_test.go because it would hit almost entirely the same code as the matcher and add more IO to the unit tests. Happy to add one if it would be helpful, though.

glesica · 2020-10-20T00:08:56Z

pkg/matcher/matcher_test.go

-	assert.Contains(t, got.Diff, "+0")
-	assert.Contains(t, got.Diff, "-1")
+	assert.Contains(t, got.Diff, "+3")
+	assert.Contains(t, got.Diff, "-2")


I noticed that there was always a "-1" in the diff output because it tells the user how many lines changed, so this makes the test a tiny bit less brittle.

dylanhitt

I really like this MR already. This is something i've personally been wanting since I start contributing and haven't gotten around to it.

One thing: if you don't mind adding an example case in examples/. You can follow the same idea as the big_out.txt and add _fixtures/ and place whatever.txt underneath.

Cheers

dylanhitt · 2020-10-20T00:37:05Z

integration/unix/commander_test.yaml

+    command: cat ./integration/unix/_fixtures/big_out.txt >&2
+    stderr:
+      file: ./integration/unix/_fixtures/big_out.txt
+
  it should inherit from parent env:


Would you mind adding a failing case as well?

dylanhitt · 2020-10-20T00:46:42Z

pkg/matcher/matcher_test.go

+
+func TestFileMatcher_Validate(t *testing.T) {
+	m := FileMatcher{}
+	got := m.Match("zoom", "zoom.txt")


I'm okay with reading directly from the filesystem here, there's not really a good way to mock ioutil.ReadFile, that I know of. The best thing I've found is this, and even this would require some code change. I personally think this is too much for well tested code already.

Another direction that could be taken is removing the actual code needing to be tested and placing it in a private helper function. For example, it wouldn't be stretch to write a helper function that could be used in both FileMatcher and TextMatcher, and just perform the I/O in the FileMatcher and then call the helper method. I'm neither here nor there on this approach.

One thing @SimonBaeumer if we take the approach from reading from the FS should we designate a folder to do this?

Or a temp file could be created and then used/removed.

I thought about the temp file approach but I wanted input since that didn't feel great either. Ordinarily I'd probably accept a Reader instead of a filename but then we're back to the original question of where does the file actually get opened.

Right, the only other thing I can really think of is to FileMatcher take an argument for a function, that basically wraps ioutil.ReadFile(filePath string). Like:

type TestingMocking struct { readFile func(filePath string) ([]byte, error) }

You could then setup FileMatcher in NewMatcher.
And then:

func (m FileMatcher) Match(got interface{}, expected interface{}) MatcherResult { buf, err := m.readFile(expected.(string)) // rest goes here

This would allow us to mock readFile. Again I think this may be too robust. I think I'll defer @SimonBaeumer on this as I don't really have preference.

Good call, I'll try that out and see how it ends up.

Hey Together,
so yes, ideally we would mock the filesystem but this not often done and not necessary imho.
I am totally fine with using libraries and access to filesystem or env variables.

The downside of mocking is in general that it gets very complex and often takes a lot of effort to maintain.
Mocking is nice if there is some complex things I want to replace, like network connections, tasks that need a lot of time or simply complex stuff to setup, i.e. you would need to run a kubernetes cluster.

Go ignores directories starting with underscores, like _assets and directories matching testdata.
Just put the asset inside a pkg/matcher/testdata directory.
You should be able access it like this:

wd, _ := os.Getwd() asset := path.Join(wd, "testdata/file.txt") content, _ := ioutil.ReadFile(asset)

The testing framework of go will execute all tests with the working directory pointing to the currently executed file, so it is easy to access the testdata.

Mocking:

Another option like @dylanhitt mentioned is to use dependency injection. That means that we inject (normally as input parameters in the constructor or functions) dependencies on other structs (or classes).
These classes / structs do implement an interface, therefore you could replace the implementation inside your tests with a fake one.

Original implementation, i.e. in file_matcher.go:

type FileReaderInterface interface { func Read(file string) string } type FileReader struct {} func (r *FileReader) Read (file strinng) string { ioutil.ReadFile... // direct access to filesystem } type FileMatcher struct { Reader FileReaderInterface } func NewFileMatcher() *FileMatcher { var reader FileReaderInterface reader = FileReader{} return &FileMatcher{ Reader: reader, } }

In your test file_matcher_test.go you would than do something like this:

type FileReaderMock {} func Read(file string) string { if file == "test.txt" { return "Mocked this for a test" } if file == "test2.txt" { return "mocked for another test" } // maybe panic if the file wasn't mocked panic("file was not mocked") } func TestFileMatcher(t *testing.T) { var readerMock FileReaderInterface readerMock = FileReaderMock{} // Inject the reader mock into the FileMatcher matcher := FileMatcher{Reader: readerMock} }

Nevertheless, I am totally happy with both approaches.

glesica · 2020-10-20T20:09:18Z

I added a mutable function to the matcher package to allow the ReadFile function to be overridden. I don't love it, but it's a lot simpler than injecting the function from higher up, particularly since it is only used for tests.

dylanhitt

This looks good to me. The last thing is to update the README to have a bullet in Table of contents under Documentation.Tests.stdout and add short example/definition similar to say xml.

We have also found it very useful to add an example in examples/. You can follow the examples/_fixtures pattern for your fixtures. That's all from me 😄

pkg/matcher/matcher.go

codeclimate · 2020-10-21T01:22:41Z

Code Climate has analyzed commit 571c5e4 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 80.0% (50% is the threshold).

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

View more on Code Climate.

glesica · 2020-10-22T14:32:00Z

I think this is ready to merge unless anyone has comments (which I'd be happy to address). Thanks for all the discussion and suggestions so far!

SimonBaeumer · 2020-10-23T06:55:18Z

examples/commander.yaml

Is this example still intended to work?
I am not sure the relative path to the fixtures would work

So the reason why this is needed is because we're not properly mocking ioutil.Readfile in the unit tests for app.TestCommand... 100% my fault. Here is the exact line. I opened #149 for this already and pasted the failing build that showed why this pathing was need.

I can move it over to the testdata approach this afternoon. So the pathing can be fixed here.

ahhh okay, than it can be merged 👍

SimonBaeumer · 2020-10-23T06:55:58Z

LGTM, just the example needs a little adjustment to the relative path or removing the file assertion.

Otherwise @dylanhitt feel free to merge

glesica · 2020-10-23T15:23:56Z

Hmm, sorry, I'm not following. Which example needs to be updated? The one in the README? Should those be runnable?

SimonBaeumer · 2020-10-23T16:57:49Z

I think this is ready to merge unless anyone has comments (which I'd be happy to address). Thanks for all the discussion and suggestions so far!

See here: https://github.com/commander-cli/commander/pull/147/files#r510672628
Can be merged, no further action required :)

dylanhitt · 2020-10-23T17:19:57Z

Merged!

glesica · 2020-10-27T02:59:48Z

Hey how would ya'll feel about doing a minor version release? I want to use the file matcher but I don't want to make people download a Go toolchain to install it.

SimonBaeumer · 2020-10-27T07:43:57Z

Hey how would ya'll feel about doing a minor version release? I want to use the file matcher but I don't want to make people download a Go toolchain to install it.

Of course 👍

dylanhitt requested review from SimonBaeumer and dylanhitt October 15, 2020 04:20

SimonBaeumer reviewed Oct 18, 2020

View reviewed changes

dylanhitt reviewed Oct 18, 2020

View reviewed changes

Implement file matcher / assertion

781486a

glesica force-pushed the add-file-assertion branch from 6916ee8 to 781486a Compare October 20, 2020 00:02

Merge branch 'master' into add-file-assertion

b282aa0

glesica commented Oct 20, 2020

View reviewed changes

dylanhitt reviewed Oct 20, 2020

View reviewed changes

glesica added 2 commits October 19, 2020 23:19

Add a failing test case and dedicated fixtures

fe89129

Add validator test and allow fake ReadFile

2a616dc

dylanhitt reviewed Oct 20, 2020

View reviewed changes

pkg/matcher/matcher.go Show resolved Hide resolved

glesica added 3 commits October 20, 2020 18:15

Add example and update readme

5b1bcbb

Fix path

adeb098

Oops, fix path again

571c5e4

SimonBaeumer reviewed Oct 23, 2020

View reviewed changes

dylanhitt approved these changes Oct 23, 2020

View reviewed changes

dylanhitt merged commit 10db468 into commander-cli:master Oct 23, 2020

glesica deleted the add-file-assertion branch October 23, 2020 17:27

SimonBaeumer mentioned this pull request Oct 24, 2020

Generate test files/dirs in app tests #150

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add file assertion #147

Add file assertion #147

glesica commented Oct 15, 2020 •

edited by dylanhitt

Loading

dylanhitt commented Oct 16, 2020

SimonBaeumer Oct 18, 2020 •

edited

Loading

glesica Oct 19, 2020

SimonBaeumer Oct 18, 2020 •

edited

Loading

glesica Oct 19, 2020

dylanhitt left a comment

dylanhitt commented Oct 18, 2020

SimonBaeumer commented Oct 18, 2020 •

edited

Loading

glesica commented Oct 20, 2020

glesica Oct 20, 2020

dylanhitt left a comment

dylanhitt Oct 20, 2020

dylanhitt Oct 20, 2020 •

edited

Loading

dylanhitt Oct 20, 2020 •

edited

Loading

glesica Oct 20, 2020

dylanhitt Oct 20, 2020

glesica Oct 20, 2020

SimonBaeumer Oct 23, 2020 •

edited

Loading

glesica commented Oct 20, 2020

dylanhitt left a comment

codeclimate bot commented Oct 21, 2020

glesica commented Oct 22, 2020

SimonBaeumer Oct 23, 2020

dylanhitt Oct 23, 2020 •

edited

Loading

SimonBaeumer Oct 23, 2020

SimonBaeumer commented Oct 23, 2020

glesica commented Oct 23, 2020

SimonBaeumer commented Oct 23, 2020

dylanhitt commented Oct 23, 2020

glesica commented Oct 27, 2020

SimonBaeumer commented Oct 27, 2020

Add file assertion #147

Add file assertion #147

Conversation

glesica commented Oct 15, 2020 • edited by dylanhitt Loading

Checklist

dylanhitt commented Oct 16, 2020

SimonBaeumer Oct 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimonBaeumer Oct 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dylanhitt left a comment

Choose a reason for hiding this comment

dylanhitt commented Oct 18, 2020

SimonBaeumer commented Oct 18, 2020 • edited Loading

glesica commented Oct 20, 2020

Choose a reason for hiding this comment

dylanhitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dylanhitt Oct 20, 2020 • edited Loading

Choose a reason for hiding this comment

dylanhitt Oct 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimonBaeumer Oct 23, 2020 • edited Loading

Choose a reason for hiding this comment

glesica commented Oct 20, 2020

dylanhitt left a comment

Choose a reason for hiding this comment

codeclimate bot commented Oct 21, 2020

glesica commented Oct 22, 2020

Choose a reason for hiding this comment

dylanhitt Oct 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimonBaeumer commented Oct 23, 2020

glesica commented Oct 23, 2020

SimonBaeumer commented Oct 23, 2020

dylanhitt commented Oct 23, 2020

glesica commented Oct 27, 2020

SimonBaeumer commented Oct 27, 2020

glesica commented Oct 15, 2020 •

edited by dylanhitt

Loading

SimonBaeumer Oct 18, 2020 •

edited

Loading

SimonBaeumer Oct 18, 2020 •

edited

Loading

SimonBaeumer commented Oct 18, 2020 •

edited

Loading

dylanhitt Oct 20, 2020 •

edited

Loading

dylanhitt Oct 20, 2020 •

edited

Loading

SimonBaeumer Oct 23, 2020 •

edited

Loading

dylanhitt Oct 23, 2020 •

edited

Loading