overview for --tree

[thorehusfeldt]

Weakly-held suggestions for the visual appearance:

1. Consider reintroducing a decading or pentadic separator, like AAAAA AATAA TRR
2. The lower case letters are redundant for sample and should just be upper case.
3. On a light terminal background, the tree edges are light gray on cream. Largely invisible.

[thorehusfeldt]

This is both mesmerising and extremely useful. Well done!

[mzuenni]

1. is in the work (right now it should more or less render like in master)
2. and 3. should be better now (can you test this?)

[RagnarGrootKoerkamp]

I probably won't have time to review until Friday.

[RagnarGrootKoerkamp]

The code looks a bit hard to review in detail, but everything seems to work well. Do you think I should check things in detail? The biggest part of the diff seems moved so should be fine probaly.

I do get some flickering but I suppose that's inevitable.

My main issue is that now the lightblack doesn't have much contrast on my dark gray (not quite black) background :sweat_smile:

[mzuenni]

argh why is there not just grey...

[RagnarGrootKoerkamp]

colours are good for me now.

Do you just want to merge? I don't really feel like checking the code iitself n detail. It seems to work well in my testing.

[mzuenni]

I will play around a bit with colors... For some reasons my terminal only displays Style.DIM differently if a color is provided... but white does not work for bright background and lightblack seems to be too dark...

[RagnarGrootKoerkamp]

Ah also note that DIM is a style and LIGHTBLACK is a colour, so that the 'expected AC' are dim-green in one and normal-lightblack in the other (ie both style and colour is different). I think I slightly prefer the dim-green over white, so the verdict column is always coloured.

---

Separately, maybe we can query terminal background colour (via terminfo or so?) and decide based on that? Probably overkill though.

[mzuenni]

Yea, i (hopefully) only changed places where "DIM + no color" or "DIM + WHITE" was used. IMO both should just be grey but that is easier said then done ^^'

[thorehusfeldt]

I have some comment in the Code about the type of the testcase parameter. (Not sure if they are visible to others; they are flagged as pending changes and I'm only on my phone's GitHub app and not sure what that means.)

[mzuenni]

I can not see them... or at least i dont know where to find them ^^'

[thorehusfeldt]

Copypasting here. (I feel like an idiot, there must be some button I‘m missing. Later: Ah! Add your review. Makes sense.)

This is line 156.

—— I don’t like like testcases for both of these things. Consider retaining testcase_list for the parameter or introducing testcase_names for the local variable. (The pylint linter also complains.) Consider this a weakly held opinion.

(Avoiding this, and simultaneously reducing dependencies by being minimal about which information is passed to the Verdicts constructor, was the reason for having the parameter testcases: list[str] in the first place.)

Also, for symmetry with the following line and calmness, consider

testcases = set(t.name for t in testcases)

—

To expand on this, is

import testcase

ever needed? Verdict.__init__ immediately forgets the Testcase object and remembers only the (full) testcase name. (This is the right thing to do, I believe.) Similarly, TableProgressBar.start only cares about names, not objects. (This is how it should be, I believe.)

[thorehusfeldt]

It was decided to increase the dependency of verdicts.py on testcase.py. Then consider for a moment the signature of

verdicts.Verdict.set(self, testcase: str, verdict: str | Verdict, duration: float)

and whether you want to overload

def __getitem__(self, testnode) -> Verdict | None | Literal[False]:

In the original architecture, verdicts conceptually map str (not testcase.Testcase) to verdicts.Verdict. Such str are test node names, which include test cases and test nodes. This was quite clean, and there’s no difference between verdict["a/b"] if "a/b" identifies a testcase or a test group (including the root). With the new architecture, it can be argued that set should take testcase.Testcase, not str (and that __getitem__ maybe takes a union type? Dunno.

In any case, the documentation is now wrong, at least there:

  Testcases and testgroups are identified by strings.  In particular,
    * the testcase whose input file is 'a/b/1.in' is called 'a/b/1'
    * the two topmost testgroups are 'sample', 'secret'
    * the root is called '.'

The need to introduce MockTestcase in test/test_verdicts.py also give me pause.

[mzuenni]

I think using a string as key is perfectly fine and makes sense. My main argument is that if we want to handle anything about testcases wr should at some point have access to the testcase.Testcase, i.e. in the init you get the testcases, extract whatever you need and afterwards provide any interface.

I also think that the comment still sounds right. Functions like set use the name to identify testcases and Testgroup and its initialized with all testcases (and not just testcase names).

[mzuenni]

IMO we can merge this?