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

Test: Add coverage for Output #149

Merged
merged 2 commits into from
Oct 2, 2024
Merged

Test: Add coverage for Output #149

merged 2 commits into from
Oct 2, 2024

Conversation

alex-kar
Copy link
Contributor

@alex-kar alex-kar commented Sep 15, 2024

@zeroshade

  1. I've figured out how to run tests with pterm, but there are a few questions I need your input on.
var buf bytes.Buffer
pterm.SetDefaultOutput(&buf)

text{}.DescribeTable(table)

// compare buf.String() with expected result

or

pterm.SetDefaultOutput(os.Stdout)

text{}.DescribeTable(table)

// Output:
// Expected output
  1. Also we need to disable colors. Otherwise output has ANSI codes, making comparison impossible.
pterm.DisableColor()
  1. Not sure if we can reuse Examples.
    testing: examples with trailing whitespace in multiple lines of output can be confusing golang/go#26460
The trailing spaces cannot be handled by the // Output: of an Example

In our case, Metadata location | has whitespace after the pipe |, which is getting removed.

// Output:
// Table format version | 2
// Metadata location    |
// ...

And also Examples do not provide any diff when actual result doesn't match expected one.
So, for this matter I used 3rd party github.com/MarvinJWendt/testza, but maybe there is a better approach.

I've pushed some working version as an example of what I have so far. I'd appreciate some feedback before I proceed with covering all scenarios.

@zeroshade
Copy link
Member

This is awesome for a find, thanks for this! As for your questions:

I personally prefer this one:

pterm.SetDefaultOutput(os.Stdout)

text{}.DescribeTable(table)

// Output:
// Expected output

But given the 3rd part of this, it can definitely make more sense to do the first option if that's easier to implement.

  1. Also we need to disable colors. Otherwise output has ANSI codes, making comparison impossible.

Can we only disable the colors for the tests? I personally don't want to lose the colors for regular output and use of the CLI, perhaps we make an option for disabling colors or a global var that the test can set to disable it?

In our case, Metadata location | has whitespace after the pipe |, which is getting removed.

Should we modify the output to not print trailing whitespaces?

And also Examples do not provide any diff when actual result doesn't match expected one.

That's fair, we just get a "want: ...." and "got: ...." which would make diffing difficult. In light of this, it might make sense to go with the example you've provided here that writes to a buffer and we compare manually so that we can output the diff instead.

@alex-kar
Copy link
Contributor Author

@zeroshade

Can we only disable the colors for the tests? I personally don't want to lose the colors for regular output and use of the CLI, perhaps we make an option for disabling colors or a global var that the test can set to disable it?

It's something that pterm provides us out of the box. We just tern global variable to disable colors in test only.
Please, see in my test example

pterm.SetDefaultOutput(&buf)
pterm.DisableColor()

text{}.DescribeTable(table)

@zeroshade
Copy link
Member

@alex-kar oh perfect! I missed that. Nice!

@alex-kar
Copy link
Contributor Author

Also I tried to write something like "parameterized" test to provide different test cases, but not sure if arguments are readable.
Here is the idea:

var testArgs = []struct {
	meta     string
	expected string
}{
	{`<metadata json 1>`, `<expecte output 1>`},
	{`<metadata json 2>`, `<expecte output 2>`},
	{`<metadata json 3>`, `<expecte output 3>`},
}

func TestDescribeTable(t *testing.T) {
	var buf bytes.Buffer
	pterm.SetDefaultOutput(&buf)
	pterm.DisableColor()
	for _, tt := range testArgs {
		meta, _ := table.ParseMetadataBytes([]byte(tt.meta))
		table := table.New([]string{"t"}, meta, "", nil)

		text{}.DescribeTable(table)

		testza.AssertEqual(t, tt.expected, buf.String())
	}
}

But because "metadata json" and "expected output" are quite long multiline strings, it make arguments less readable.

@zeroshade WDYT? Is it better to just write separate tests for each case or go with different parameters for the same test?
However, if we use parameters, we won't be able to reuse Examples.

@alex-kar alex-kar changed the title Test: Add coverage for Output (WIP) Test: Add coverage for Output Sep 18, 2024
@alex-kar
Copy link
Contributor Author

@zeroshade Please review tests to cover output.
I added two parameters: one with minimal table metadata and another with all fields set. For comparison, I used github.com/stretchr/testify/assert, which is already in the project and provides a nice diff view.

@alex-kar alex-kar marked this pull request as ready for review September 18, 2024 01:47
Copy link
Member

@zeroshade zeroshade left a comment

Choose a reason for hiding this comment

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

LGTM thanks!

@nastra nastra merged commit d4bdb63 into apache:main Oct 2, 2024
9 checks passed
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.

3 participants