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

[STACKED] [WIP] [UX feedback requested]: testing logs user experience #4080

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

apostasie
Copy link
Contributor

@apostasie apostasie commented Apr 4, 2025

<!> Blocked by <!>:


This PR focuses specifically on enhancing the human experience of looking at this:

Untitled

I do spend a lot of time debugging on the CI, or locally, and here are my issues with this:

It is all packed, and very hard to distinguish one line from the other - everything visually stands at the same level - headers, context information, failure, file and line numbers.

This here is actually a simple failure - for more complex ones, it literally takes minutes of squinting to figure out where is the command that was run, and more time to figure out what is the "failure".

More often than not, the failure is really cryptic, as in false is not true.
Yeah, no shit Sherlock.
So, you do have to context switch to an IDE, find the file, go to that line number, try to understand the test, read the tea leaves, etc.
Also, since the filename is shown as basename only, I regularly open the wrong (identically named) one.

Context is hard to figure out too.

There are empty sections ("Setup") - did anything run in there? or not? if nothing ran, why show the section?
If something ran, what was it so that I can reproduce the context before trying to debug the command.

Env is a can of worms on its own - useful, but also very loud.

This PR would like to propose we address all these issues:

  • separate things so that they are easier to visually spot
  • hide what is not necessary, show what is
  • show full lifecycle context to ease manual reproduction
  • use indentation, tables, colors, borders, arrows, emoji, kitchen sink if necessary, in order to make this thing easier to visually grok and navigate fast
  • hyperlinks where feasible to quickly open the right file in your IDE instead of searching around
  • clear failure information, what was actual, what was expected, what did it all mean?
  • clear sections emphasizing lifecycle

I do appreciate that everyone has their own tastes and preferences when it comes to visual things, so, I do not expect that we are going to agree on every detail, and we will have to settle on something.

The important part here is to come up with something that objectively addresses these issues - then we can bikeshed all we want on pink vs. green :-) (or I will make the output configurable so that different projects can pick their poison).

I truly believe very few people engage with the CI failures not because they are lazy, but because it looks like a train accident.

PLEASE NOTE THAT THIS PR CODE IS NOT READY FOR REVIEW.
What is ready for review and discussion right now is the visual end-result (see next comment).

apostasie added 17 commits April 3, 2025 09:26
Signed-off-by: apostasie <[email protected]>
- use internal/com instead of icmd
- move pty from test/internal to internal
- update go mod, sum, and depguard

Signed-off-by: apostasie <[email protected]>
- Background() signature change
- command creation change

Signed-off-by: apostasie <[email protected]>
- Background
- Feed, WithFeeder, WithPseudoTTY
- use exit code consts where appropriate
- some --quiet

Signed-off-by: apostasie <[email protected]>
This commit relaxes line length from 100 to 120, and disables a bunch of linters we do not want.

Signed-off-by: apostasie <[email protected]>
Mimicry is a lightweight, zero dependency mock mechanism created to ease testing of Tigron.
Since Tigron heavily relies on *testing.T, it is currently hard to test.
Moving away to a tig.T interface instead of *testing.T will unlock the ability to mock.
Mimicry does provide:
- recording of all function calls, with arguments and complete stack trace (see Report())
- optional custom handling of function calls (see Register())
- QOL: fancyfied OCS8 links allow opening files from traces in terminal

UX is largely in flux right now and experimental, but the objective is to:
- do not require code generation
- do not abuse reflection
- keep the amount of boilerplate to the absolute minimum for the mock consumer
- ... and as small as possible for the mock creator
- keep zero dependencies

This commit also introduce the tig.T interface to be used everywhere inside Tigron in the future,
along with a complete mock for it.

Mimicry is not meant to be used directly for now, though, if there is interest, a future
version might graduate out of `internal`.

Signed-off-by: apostasie <[email protected]>
This brings a set of enhancements to assertive:
- function name simplifications
- generics-ifying of some functions that can be used on comparables
- additional methods (Match, DoesNotMatch)
- debugging output is made clear, along with OCS8 link and excerpt of the test file
- all methods can now elect to Fail later instead of FailNow
- Check method is removed (^ because of above)
- all methods can now elect to silence output on success

Other Tigron files are updated to adopt these changes:
- method names update
- comparators move away from Check and now fully leverages assertive

This also adds a large number of tests for assertive, leveraging mimicry.

Signed-off-by: apostasie <[email protected]>
Prior, only BlackList-ing of environment variables was supported, and solely for exact variable names match, or "*" for all.

This changeset introduces:
- WhiteList-ing
- prefix matching for env var names (eg: "THING_*" can now be used to white/black list any env variable which name starts with THING_)

Signed-off-by: apostasie <[email protected]>
This is mostly a cosmetic changeset:
- refined golangcilint, explicitly calling which linters we really care about, along with settings for revive
- comments on reasons for some of //nolint
- comments line breaks
- additional FIXME information
- assert type check fixes
- const-ification
- overall making linter happy(-ier)
- some tests enhanced with added info

And:
- adding a WhiteList test for internal/com

Signed-off-by: apostasie <[email protected]>
This changeset adds expect.JSON[T any], which will allow easier testing of json output and hopefully
remove a lot of boilerplate unmarshalling / assert in tests.

It also adds an extensive doc.md document about comparators (plan is trim down the main documentation
to a small set of simple examples, then link to these "advanced" docs for further reading), which
will allow for easier documentation maintenance and more approachable reading.

Signed-off-by: apostasie <[email protected]>
In some cases, exec is just really slow.
Adjusting tests so that we start counting after the command actually started.

Signed-off-by: apostasie <[email protected]>
Turns out exec could be really slow in certain circumstances, and we should only measure time once the command is done starting.

Signed-off-by: apostasie <[email protected]>
@apostasie
Copy link
Contributor Author

apostasie commented Apr 4, 2025

The current proposal of this PR is:

Screenshot 2025-04-03 at 8 52 42 PM

@apostasie
Copy link
Contributor Author

apostasie commented Apr 4, 2025

Here is the text version of that same failure, copy-pasted from my terminal.

   volume_inspect_test.go:193:
        ⤵️️ "TestVolumeInspect": into subtests prep
    volume_inspect_test.go:193:
        ↩️️ "TestVolumeInspect": done with subtests prep
    --- FAIL: TestVolumeInspect/inspect_labels (0.09s)
        volume_inspect_test.go:193:

            +============================================================================================================+
            | 🚀       | "TestVolumeInspect/inspect_labels": starting test!                                               |
            +============================================================================================================+

        volume_inspect_test.go:193:

            +------------------------------------------------------------------------------------------------------------+
            | Command | /usr/local/bin/nerdctl volume inspect testvolumeinspect-second-744d9160                          |
            +------------------------------------------------------------------------------------------------------------+
            | Stdout  | [                                                                                                |
            |         |     {                                                                                            |
            |         |         "Name": "testvolumeinspect-second-744d9160",                                             |
            |         |         "Mountpoint": "/home/dmp.linux/.local/share/nerdctl/1935db59/volumes/nerdctl-test/testvo |
            |         | lumeinspect-second-744d9160/_data",                                                              |
            |         |         "Labels": {                                                                              |
            |         |             "bar": "barval",                                                                     |
            |         |             "foo": "fooval"                                                                      |
            |         |         }                                                                                        |
            |         |     }                                                                                            |
            |         | ]                                                                                                |
            |         |                                                                                                  |
            |         |                                                                                                  |
            +------------------------------------------------------------------------------------------------------------+

        volume_inspect_test.go:193:

            <<<<<<<<<<<<<<<<<<<<
            	🖊️ Inspecting output (contains)
            	👀 testing:		`[
                {
                    "Name": "testvolumeinspect-second-744d9160",
                    "Mountpoint": "/home/dmp.linux/.local/share/nerdctl/1935db59/volumes/nerdctl-test/testvolumeinspect-second-744d9160/_data",
                    "Labels": {
                        "bar": "barval",
                        "foo": "fooval"
                    }
                }
            ]

            `
            	❌ FAILED!		~= `testvolumeinspect-second-744d9160XXXXXXXXXX`
            >>>>>>>>>>>>>>>>>>>>

        volume_inspect_test.go:193:

            <<<<<<<<<<<<<<<<<<<<
            	🖊️ Unmarshalling JSON from stdout must succeed
            	👀 testing:		`<nil>`
            	✅️ does verify:		is `<nil>`
            >>>>>>>>>>>>>>>>>>>>

    volume_inspect_test.go:193:

        +============================================================================================================+
        | 🧽️      | "TestVolumeInspect": post-cleanup                                                                |
        +============================================================================================================+

    volume_inspect_test.go:72:

        +------------------------------------------------------------------------------------------------------------+
        | Command | /usr/local/bin/nerdctl volume rm -f testvolumeinspect-first-15e4b5b2                             |
        +------------------------------------------------------------------------------------------------------------+
        | Stdout  | testvolumeinspect-first-15e4b5b2                                                                 |
        |         |                                                                                                  |
        +------------------------------------------------------------------------------------------------------------+

    volume_inspect_test.go:73:

        +------------------------------------------------------------------------------------------------------------+
        | Command | /usr/local/bin/nerdctl volume rm -f testvolumeinspect-second-744d9160                            |
        +------------------------------------------------------------------------------------------------------------+
        | Stdout  | testvolumeinspect-second-744d9160                                                                |
        |         |                                                                                                  |
        +------------------------------------------------------------------------------------------------------------+

FAIL
FAIL	github.com/containerd/nerdctl/v2/cmd/nerdctl/volume	0.667s
FAIL

Following on assertive debugging info cleanup, this revamp how we display failures.
- previously, every assert failure would receive the entire execution information  - we no longer do that and instead print out command information first, separately
- a lot of headers were printed about different stages (setup, cleanup), even when there was nothing done in these stages, resulting in a lot of noise
- the new display embraces a more structured, tabled output, that should significantly enhance legibility

Signed-off-by: apostasie <[email protected]>
@apostasie apostasie changed the title [UX feedback requested]: debugging user experience [UX feedback requested]: testing logs user experience Apr 4, 2025
@apostasie apostasie marked this pull request as ready for review April 4, 2025 20:25
@AkihiroSuda
Copy link
Member

Thanks, the output looks better now

@apostasie apostasie changed the title [UX feedback requested]: testing logs user experience [STACKED] [WIP] [UX feedback requested]: testing logs user experience Apr 8, 2025
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