test(table): add test for consistent table-column-width

[hasezoey]

This PR adds a test for widget Table to test that the widths for columns are consistent, added this test because i have found inconsistencies when trying #395, see #370 (comment)

[codecov]

Codecov Report

Merging #404 (ed17185) into main (f4ed3b7) will decrease coverage by 0.53%.
The diff coverage is 100.00%.

❗ Current head ed17185 differs from pull request most recent head 48e4953. Consider uploading reports for the commit 48e4953 to get more accurate results

@@            Coverage Diff             @@
##             main     #404      +/-   ##
==========================================
- Coverage   86.14%   85.62%   -0.53%     
==========================================
  Files          40       40              
  Lines        9256     8982     -274     
==========================================
- Hits         7974     7691     -283     
- Misses       1282     1291       +9     

Files Changed	Coverage Δ
src/widgets/table.rs	94.90% <100.00%> (+1.72%)	⬆️

... and 3 files with indirect coverage changes

[joshka]

The main thing I notice with this test is that it's testing a lot of things at once with more code than seems necessary to just test the behavior it's interested with. Let's try to simplify the test (perhaps by splitting this into multiple tests if that would help).

Observations:

• unsure what "consistent" means in this context. Use a name that describes this better.
• multiple rows, but nothing seems dependent on rows. Remove all but the first and use whatever test data makes sense
• 3 cells in each row, but 5 constraints. Add cells or comment on why this is necessary
• large numbers that don't seem obvious why those particular numbers were chosen. Use smaller numbers that more easily show the properties being tested.
• only testing one particular combination of constraints - unsure what the significance of this combination is. I think this test just tests that if there is enough space to render all the columns then all the column widths will be full (?)
• no point testing sums when the values that are being added are already tested

How about testing this by making a small function that returns the widths of a table with given constraints. E.g.:

fn get_column_widths() {
  use ratatui::layout::Constraints::*;
  fn test(widths: &[Constraint], width: u16, selection_width: u16) {
    let table = Table::new(vec![Row::new(vec!["test"; constraints.len()])])
        .widths(widths);
    table.get_column_widths(width, selection_width)
  }

  assert_eq!(test(&[Min(0)], 10, 0), &[(0, 10)], "min(0)");
  assert_eq!(test(&[Max(5)], 10, 0), &[0, 5)], "max(5)");
  ...
}

[hasezoey]

only testing one particular combination of constraints - unsure what the significance of this combination is. I think this test just tests that if there is enough space to render all the columns then all the column widths will be full (?)

this particular combination of 5 constraints came to be because

• at first it was just the layout.rs example constraints
• then i ran into Table: Add option to make expand_to_fill configurable for Table::widths #370
• then i added another (infinitely) growable constraint and noticed Table: Add option to make expand_to_fill configurable for Table::widths #370 (comment)

so at the end i guess this is testing a combination of all constraints at least 2 growable columns

unsure what "consistent" means in this context. Use a name that describes this better.

could you maybe explain what line you mean / context? i am pretty sure the comments like constraint Length(4) should be obvious to the constraint in the widths

[hasezoey]

are those tests better? (de96ea0)

[joshka]

are those tests better? (de96ea0)

Yep. These are a good set of characterization tests. This will make it easier to make changes in this area and know if we break things (or change things in ways that are planned). I like the module layout you went with too.

For the last test, what I'm looking for is a bit more intent to be communicated from part of the test itself -something that would help a future maintainer (even if that's you in a couple of months time) understand what the purpose of the test is. The test is either an example of something that works / doesn't work right now (I'm not sure which), but it's not a minimal example. This makes it difficult to understand what the intent of the test is and which of the parts are the significant part. Is it that there are two Min constraints? Is it the combination of Min and Max? Is it the ordering? Is it the different sizes and mix of the 3 types? Could the same information be communicated with fewer Constraints (and if so, which ones)?. If we change some of the behavior and break this test doing so, how would I know whether the test is right or whether the change is right?

could you maybe explain what line you mean / context? i am pretty sure the comments like constraint Length(4) should be obvious to the constraint in the widths

I meant that the word "consistent" has several meanings, and either:

- suggests that something doesn't change (it's stable), in which case someone coming later might want to know why it might change. Possibly this was something to do with the issue that I was seeing where cassowary solves can be non-deterministic. - suggests that it is compatible with something else or itself (but the other thing wasn't mentioned in the test name or the commit message and I was looking for something that might have been constrasted)

[hasezoey]

unsure what "consistent" means in this context. Use a name that describes this better.

could you maybe explain what line you mean / context? i am pretty sure the comments like constraint Length(4) should be obvious to the constraint in the widths

I meant that the word "consistent" has several meanings, and either:

i am sorry, i misread unsure what "consistent" means in this context as unsure what "constraint" means in this context

[hasezoey]

For the last test, what I'm looking for is a bit more intent to be communicated from part of the test itself

i have removed it for now, because it is actually now covered by min_constraint (to be more specific, the first 2 assert_eq of that)

[joshka]

LGTM. Thanks for the PR. Can you squash this into a single commit please?

[hasezoey]

rebased onto latest main and squashed commits

had to change one test line though, this test had to be changed from

// without selection, less than needed width
assert_eq!(
    get_test(&[Constraint::Min(4), Constraint::Min(4)], 7, 0),
    &[(0, 4), (4, 3)]
);

// with selection, less than needed width
assert_eq!(
    get_test(&[Constraint::Min(4), Constraint::Min(4)], 7, 3),
    &[(0, 4), (4, 3)]
);

into

// without selection, less than needed width
assert_eq!(
    get_test(&[Constraint::Min(4), Constraint::Min(4)], 7, 0),
    &[(0, 4), (3, 4)]
);

// with selection, less than needed width
assert_eq!(
    get_test(&[Constraint::Min(4), Constraint::Min(4)], 7, 3),
    &[(3, 4), (3, 4)]
);

[joshka]

Can you explain the difference to that test? Would this have been because of the refactoring changes in #405 ? In which case, that was unintentional?

[hasezoey]

i am pretty sure it is a side-effect of #405, though i am not sure which exact change caused it.
the difference now is that:

• for the first mentioned one, no spacer is allocated anymore, but both constraints are fullfilled through 0 to 7, resulting in 8 characters drawn, without spacer (even though there should probably be a spacer)*¹
• for the second one, i guess it actually allocated space for the selection width this time and somehow made the following constraints allocate the same space and so overwrite each other (see the same starting x(the first in the tuple) coordinate)

*¹ i am not sure if it is correct that max_width is +1 after 0 instead of -1 (like a .len() would be done)

some example from before / after #405

(testing 2 constraints of "Min(4)", no selection width)

after #405
0 width:
[(0, 0), (0, 0)]
1 width:
[(0, 1), (0, 1)]
2 width:
[(0, 2), (0, 2)]
3 width:
[(0, 3), (0, 3)]
4 width:
[(0, 4), (0, 4)]
5 width:
[(0, 4), (1, 4)]
6 width:
[(0, 4), (2, 4)]

before #405
0 width:
[(0, 0), (0, 0)]
1 width:
[(0, 1), (1, 0)]
2 width:
[(0, 2), (2, 0)]
3 width:
[(0, 3), (3, 0)]
4 width:
[(0, 4), (4, 0)]
5 width:
[(0, 4), (4, 1)]
6 width:
[(0, 4), (4, 2)]

[joshka]

Thanks - that was really helpful. A minimal repro cutting the table out of the loop and just focusing on the layout is:

            let layout = Layout::default()
                .constraints(vec![Min(1), Length(0), Min(1)])
                .direction(Direction::Horizontal)
                .split(Rect::new(0, 0, 1, 1));
            assert_eq!(
                layout[..],
                [
                    Rect::new(0, 0, 1, 1),
                    Rect::new(1, 0, 0, 1),
                    Rect::new(0, 0, 1, 1), // x should never start before previous x
                ]
            )

[hasezoey]

rebased onto latest master, fixed the test (thanks to #410) and added a doc-comment for get_test

[joshka]

For the issue of the spacer going missing - this happens because the spacers have equal weighting as the columns.
There's two ways we could fix this:

• add optional weights to the constraints to make the spacers higher priority
• add a configurable explicit gap between chunks (though how configurable this should be would be a question to explore).

[hasezoey]

For the issue of the spacer going missing - this happens because the spacers have equal weighting as the columns.

i think this is somewhat out-of-scope for this PR, though yes something like configurable priorities would be great for such a case

[joshka]

Thanks for this PR and the patience to continue on it while there were a few other things in flux around it. Much appreciated!