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

export format property variables #347

Merged
merged 7 commits into from Aug 28, 2019
Merged

export format property variables #347

merged 7 commits into from Aug 28, 2019

Conversation

mitchdraft
Copy link
Contributor

Usually 5 characters of context around the diff location is enough, but it would be nice to be able to change that value as needed

#346

@mitchdraft
Copy link
Contributor Author

I also exported the truncateThreshold while I was at it, though I don't have a personal interest in customizing that value at the moment

@mitchdraft
Copy link
Contributor Author

I think the test failure is a flake

$ go get -v ./...
Fetching https://golang.org/x/net/html/charset?go-get=1
https fetch failed: Get https://golang.org/x/net/html/charset?go-get=1: dial tcp 172.217.212.141:443: i/o timeout
package golang.org/x/net/html/charset: unrecognized import path "golang.org/x/net/html/charset" (https fetch: Get https://golang.org/x/net/html/charset?go-get=1: dial tcp 172.217.212.141:443: i/o timeout)
Fetching https://gopkg.in/yaml.v2?go-get=1
Parsing meta tags from https://gopkg.in/yaml.v2?go-get=1 (status code 200)
get "gopkg.in/yaml.v2": found meta tag get.metaImport{Prefix:"gopkg.in/yaml.v2", VCS:"git", RepoRoot:"https://gopkg.in/yaml.v2"} at https://gopkg.in/yaml.v2?go-get=1
gopkg.in/yaml.v2 (download)
github.com/golang/protobuf (download)
The command "go get -v ./..." failed and exited with 1 during .
Your build has been stopped.

It passed on go 1.11.x and go 1.12.x

@williammartin
Copy link
Sponsor Collaborator

Retriggered

@williammartin
Copy link
Sponsor Collaborator

This makes sense to me but it would be great if we could get some test coverage on the change.

@mitchdraft
Copy link
Contributor Author

I added some tests @williammartin @blgm - let me know if you'd like any changes

Copy link
Collaborator

@blgm blgm left a comment

Choose a reason for hiding this comment

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

Hi @mitchdraft. Thank you for adding these tests. I've suggested a few small tweaks that I think makes them even better.

I notice that there aren't any tests around the limits for these values. For instance what happens when they are zero or negative? How do you feel about the risks around that? Are there some easy improvements that could be made? I see that MaxDepth is defined as type uint which means that it's impossible to set it to a negative value. Could a similar approach reduce risk here?

})
})

Context("With alternate diff lengths", func() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The context name is exactly the same as the previous test. I think it should be different and say something like with alternative number of characters to include around mismatch.

CharactersAroundMismatchToInclude = 10
Expect(MessageWithDiff(largeString, "to equal", smallString)).Should(Equal(expectedTruncatedStartSizeFailureMessageExtraDiff))
Expect(MessageWithDiff(smallString, "to equal", largeString)).Should(Equal(expectedTruncatedStartSizeSwappedFailureMessageExtraDiff))
CharactersAroundMismatchToInclude = initialValue
Copy link
Collaborator

@blgm blgm Aug 27, 2019

Choose a reason for hiding this comment

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

Other tests (the two above for example) set/unset things in a Before/AfterEach. It would be nice to match the style of the other tests as a file is easier to read and maintain if it follows a consistent style.

At the moment this test checks both the default and alternative behaviour. Perhaps the default testing should be left to the tests above, and this test should focus only on the alternative behaviour.

})

Context("With alternate diff lengths", func() {
It("long strings that differ only in length", func() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ideally BDD style tests should read as a sentence. For example the test above reads it should show the full diff, and this one could read something like it shows more characters around a line length mismatch - test names should describe the behaviour being tested rather than the implementation of the test.

@mitchdraft
Copy link
Contributor Author

thank you for the feedback @blgm, ready for another look

@blgm
Copy link
Collaborator

blgm commented Aug 28, 2019

Thank you @mitchdraft!

@blgm blgm merged commit 642e5ba into onsi:master Aug 28, 2019
@mitchdraft
Copy link
Contributor Author

sure thing! Would you mind making a new release @blgm? We've been looking forward to this feature and will use it immediately in our test utils https://github.com/solo-io/go-utils/blob/master/testutils/assertions.go#L25

@blgm
Copy link
Collaborator

blgm commented Aug 28, 2019

@mitchdraft
Copy link
Contributor Author

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants