apache / iceberg-go

Apache Iceberg - Go
https://iceberg.apache.org/
Apache License 2.0
143 stars 35 forks source link

Test: Add coverage for Output #149

Closed alex-kar closed 1 month ago

alex-kar commented 2 months ago

@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()
  2. Not sure if we can reuse Examples. https://github.com/golang/go/issues/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 commented 2 months ago

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 commented 2 months ago

@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 commented 2 months ago

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

alex-kar commented 2 months ago

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 commented 2 months ago

@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.