Allow explicitly empty body in issue/pr create#3787
Conversation
There was a race condition wherein the test didn't wait enough time for the prompt to get rendered before testing the terminal output.
pkg/cmd/pr/shared/params.go
Outdated
| if state.Body != "" { | ||
| q.Set("body", state.Body) | ||
| } | ||
| // We always want to set body, even if it's empty, to prevent the web interface to apply the default |
There was a problem hiding this comment.
A nitpick, but "...to prevent the web interface from applying the default..." sounds better than "...to prevent the web interface to apply the default...".
|
I still have concerns about the if statement that I mentioned in the other issue. This potentially still overwrites a blank body if you didn't set Also, do you think adding an if statement at the end of |
That statement is copied from upstream where it's a feature of the Survey library that we are using. I don't see our need for it, but I didn't want to delete it because I didn't want to introduce unnecessary changes that diverge from upstream. You are right that if
That would be smart, but again, we inherited this code from upstream, so best not change it without a strong reason. Also, this PR eliminates what I think is the last active special-casing of an empty body post-editing, so in doesn't matter whether we normalize whitespace or not. |
samcoe
left a comment
There was a problem hiding this comment.
This looks good to me. I like that we now have better test coverage for surveyext. The new tests did take a couple read throughs to fully understand what was happening but I couldn't think of a cleaner way to write them.
| } | ||
|
|
||
| func TestHelperProcess(t *testing.T) { | ||
| if os.Getenv("GH_WANT_HELPER_PROCESS") != "1" { |
There was a problem hiding this comment.
What is the purpose of having the GH_WANT_HELPER_PROCESS check? Seems like it is a precautionary measure, but I can't think of the case it is guarding against.
There was a problem hiding this comment.
The TestHelperProcess function needs to be skipped during normal test run. It should only be active when invoked as a subprocess
Yeah, tests for event-handling functionality are tricky because we have to have at minimum of 2 goroutines to test it. I went with a few different approaches and this was the cleanest I could come up with. |
In
issue/pr create, choosing a template but then opening your editor and deleting all contents there should be submitting a blank body, in my opinion, since you've explicitly deleted all contents. But this wasn't happening for two reasons:Body == ""as a special case that meant "user has skipped editing". (We quit doing that in Handle default body text when creating issues and pull requests #3658, but the code that appended the template to a blank body still remained.) Fixes PR body(description) is not reflecting as empty even after removing the content completely #1294&body=query parameter wasn't present, and thus the web interface would apply the template body again.Bonus:
surveyextextension is now fully tested by spawning a pseudo-TTY and interacting with Survey there.Manual testing note: to test saving a completely empty file in vim, I would have to do
:set noendofline nofixendofline, since otherwise vim always includes a trailing newline (and thus the resulting string wouldn't be empty)./cc @chemotaxis