radius-project / radius

Radius is a cloud-native, portable application platform that makes app development easier for teams building cloud-native apps.
https://radapp.io
Apache License 2.0
1.46k stars 93 forks source link

Prompt text test occasionally fails #7721

Closed brooke-hamilton closed 2 months ago

brooke-hamilton commented 3 months ago

Steps to reproduce

  1. Run make test, following instructions at contributing-code-tests.
  2. Observe the result of this test: github.com/radius-project/radius/pkg/cli/prompt/text. If the test passed, run it in isolation, without cached results. For example, run the test using this command: go test -run ^Test_E2E$/^confirm_value$ github.com/radius-project/radius/pkg/cli/prompt/text -count=1

Eventually the test will fail when running it in isolation, especially if you are on a resource constrained machine, like GitHub Codespaces. The test failed for me when walking through the initial setup instructions for contributing. When running on a fast desktop using the dev container, I was still able to get the test to fail, but it happens much less often, suggesting that there is a timing issue.

Observed behavior

When the test fails, the output will look like this:

$ go test -run ^Test_E2E$/^confirm_value$ github.com/radius-project/radius/pkg/cli/prompt/text -count=1
--- FAIL: Test_E2E (0.05s)
    --- FAIL: Test_E2E/confirm_value (0.05s)
        text_test.go:92: Testing output:

        text_test.go:92: Testing output:

test prompt 

            > test placeholder

            (ctrl+c to quit)
        text_test.go:144: 
                Error Trace:    /workspaces/radius/pkg/cli/prompt/text/text_test.go:144
                Error:          Should be empty, but was test prompt
                                > a
                Test:           Test_E2E/confirm_value
FAIL
FAIL    github.com/radius-project/radius/pkg/cli/prompt/text    0.057s
FAIL

A passing test will report a result like this:

ok      github.com/radius-project/radius/pkg/cli/prompt/text    0.057s

Desired behavior

The expected result is for the test to pass every time.

Workaround

No response

rad Version

Main branch, commit 13d50f7b02f77b868bebb9a3582953d3bc7c9f2d.

Operating system

Dev container, running in GitHub Codespaces and also the same dev container running on a desktop machine using Docker Desktop, with the source cloned to WSL (Ubuntu default).

Additional context

You can force the test to fail every time by adding a sleep command before pkg/cli/prompt/text/text_test.go (138). For example (starting on line 137):

tm.Type("abcd")
time.Sleep(1 * time.Second)
tm.Send(tea.KeyMsg{Type: tea.KeyEnter})

Suggested fix:

Later in the test method, there is this line, which is expecting the output to be an empty string. I think this line is incorrect and should be removed. The reason it usually passes is because the underlying TeaTest test model has not finished rendering the text output. The time.Sleep statement above shows how pausing execution gives the test model time to catch up and render the text. The Tea Test docs say that the final output will be available when the FinalModel method is run. This line below is run before FinalModel runs, which is why it usually, but not always, passes.

require.Empty(t, strings.TrimSpace(output)) // Output sometimes contains a single space.

In other words, I think the fix is simply to delete the line above, because the expected output is not actually an empty string, even though at that point in the code execution the console has not finished rendering and the output is often an empty string.

Would you like to support us?

AB#12635

radius-triage-bot[bot] commented 3 months ago

:wave: @brooke-hamilton Thanks for filing this bug report.

A project maintainer will review this report and get back to you soon. If you'd like immediate help troubleshooting, please visit our Discord server.

For more information on our triage process please visit our triage overview

radius-triage-bot[bot] commented 3 months ago

This issue is a great one to pickup for new contributors. It should only require small changes and not assume a deep knowledge of the Radius architecture.

We always welcome community contributions! If you would like to pick this item up sooner and submit a pull request, please visit our contribution guidelines and assign this to yourself by commenting "/assign" on this issue.

For more information on our triage process please visit our triage overview

radius-triage-bot[bot] commented 3 months ago

:+1: We've reviewed this issue and have agreed to add it to our backlog. Please subscribe to this issue for notifications, we'll provide updates when we pick it up.

We also welcome community contributions! If you would like to pick this item up sooner and submit a pull request, please visit our contribution guidelines and assign this to yourself by commenting "/assign" on this issue.

For more information on our triage process please visit our triage overview

brooke-hamilton commented 3 months ago

/assign @brooke-hamilton