Skip to content

Conversation

mislav
Copy link
Contributor

@mislav mislav commented Jan 18, 2021

For asserting command output, exact string matches are preferred in most cases. In cases when a pattern match is needed, the test can use regexp ad hoc.

For asserting command output, exact string matches are preferred in most cases. In cases when a pattern match is needed, the test can use regexp ad hoc.
@vilmibm
Copy link
Contributor

vilmibm commented Jan 19, 2021

this means having to deal with escape codes. a lot of the use of regexp is to not have to worry about escape codes, not to deal with dynamic input. i don't see this as urgent technical debt; is there a particular place where regexp testing bit us?

@mislav
Copy link
Contributor Author

mislav commented Jan 19, 2021

a lot of the use of regexp is to not have to worry about escape codes, not to deal with dynamic input.

It used to be, but we never output ANSI escape codes in tests anymore, even those that are in "TTY" mode, since iostreams.Test() always returns an IOStreams instance that has color disabled, and the ColorScheme object respects that.

Even if we were outputting ANSI escape codes, I would still suggest that we do exact matching of output after we manually stripped them. So, it would be something like:

output = stripansi.Strip(stdout.String())
assert.Equal(t, "...", output)

But I'm not sure if there is a single place in the codebase where we would need to apply such an approach right now.

i don't see this as urgent technical debt; is there a particular place where regexp testing bit us?

No, it's not an urgent technical debt. I'm trying to get us to the point where we can drop the ambiguously named test package. A lot of what ExpectLines was used for an be just as easily achieved with exact matches or with assert.Contains().

Copy link
Contributor

@vilmibm vilmibm left a comment

Choose a reason for hiding this comment

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

ah, I forgot about the newer iostreams's test instances. I'm fine with this, then.

@vilmibm vilmibm merged commit 14b8047 into cmd-stub-new Jan 21, 2021
@mislav mislav deleted the expectlines-deprecate branch January 22, 2021 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants