-
Notifications
You must be signed in to change notification settings - Fork 97
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
Unskipping tea tests and adding protection around possible nil models #8053
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8053 +/- ##
==========================================
+ Coverage 59.65% 59.80% +0.14%
==========================================
Files 577 577
Lines 38573 38573
==========================================
+ Hits 23010 23067 +57
+ Misses 13916 13856 -60
- Partials 1647 1650 +3 ☔ View full report in Codecov by Sentry. |
665240c
to
2eb7adc
Compare
2eb7adc
to
8636b76
Compare
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
pkg/cli/cmd/radinit/display_test.go
Outdated
@@ -50,6 +48,11 @@ func Test_summaryModel(t *testing.T) { | |||
waitForEmpty := func(t *testing.T, reader io.Reader) string { | |||
normalized := "" | |||
teatest.WaitFor(t, reader, func(bts []byte) bool { | |||
if bts == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a theory about why this would happen? Why is this the right fix?
pkg/cli/prompt/text/text_test.go
Outdated
tm.WaitFinished(t, teatest.WithFinalTimeout(waitTimeout)) | ||
|
||
finalModel := tm.FinalModel(t) | ||
require.NotNil(t, finalModel, "Final model should not be nil") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please explain why this is a good fix given the failure pattern from the original issue? #7670
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that there is sometimes a very short delay of model being updated. I am still in the process of proving this. The reason for my thinking is that we get a nil pointer exception, and we don't get it all the time. I think, but am not sure yet, that waiting for the model to be completely finished may fix the issue.
As mentioned above, I am working on learning more about the tea library and proving my point. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that there is sometimes a very short delay of model being updated
That's my theory as well. The test is flaky, which means the root cause is probably a data-race.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are example tests from the original repo: https://github.com/charmbracelet/x/blob/20117e9c8cd5ad229645f1bca3422b7e4110c96c/exp/teatest/app_test.go#L34.
8636b76
to
8a5c371
Compare
8a5c371
to
5194103
Compare
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
5194103
to
2173639
Compare
2173639
to
39f59c8
Compare
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
Signed-off-by: ytimocin <[email protected]>
39f59c8
to
046f346
Compare
// Wait for final render and exit. | ||
tm.WaitFinished(t, teatest.WithFinalTimeout(waitTimeout)) | ||
waitForEmpty(t, tm.FinalOutput(t)) | ||
if err := tm.Quit(); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see example tests from the original repo: https://github.com/charmbracelet/x/blob/20117e9c8cd5ad229645f1bca3422b7e4110c96c/exp/teatest/app_test.go#L34. We can discuss about them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a different test though right? We're now explicitly telling the model to quit.
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
Description
Un-skipping tea tests and adding protection around possible nil models.
Type of change
Fixes: #issue_number
Contributor checklist
Please verify that the PR meets the following requirements, where applicable: