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

Jump to bottom

x/playground: make the outcome of "Run" clearer #25454

Closed
ysmolski opened this issue May 18, 2018 · 15 comments
Closed

x/playground: make the outcome of "Run" clearer #25454

ysmolski opened this issue May 18, 2018 · 15 comments
Labels
FrozenDueToAge Proposal Proposal-Accepted
Milestone
Proposal

Comments

@ysmolski
Copy link
Member

ysmolski commented May 18, 2018 •
edited

Currently playground ends every output with "Program exited.".
It is confusing because sometimes program was not run at all.

I propose the behaviour below.

In case of success:

Hello, playground

Program exited.

Vet errors and successful run of a program:

prog.go:8: Println arg list ends with redundant newline
Go vet exited.

Hello, playground


Program exited.

In case of failed compilation:

prog.go:4:2: imported and not used: "fmt"
prog.go:8:2: undefined: qwe

Go build failed.

In case of failed test:

=== RUN   TestOne
--- FAIL: TestOne (0.00s)
got:
1
want:
2
FAIL

Program exited.

In case of program timeout. Display the recorded output before the program was killed. This might require returning both Errors and Events fields. #10590

Hello, playground
Hello, playground
...

Program was killed due timeout.

Bonus parts (implement in that does not break /compile interface):

  1. try to fit showing "Program exited with the code 3." in case of non zero exit code and no tests running.
  2. try to fit: "all tests passed" / "N tests failed" in case of tests

UP: removed exit status from proposal.
UP2: added bonus parts
@gopherbot gopherbot added this to the Proposal milestone May 18, 2018
@jimmyfrasche
Copy link
Member

Maybe something like "all tests passed" / "N tests failed" when running tests and/or examples?

@rsc
Copy link
Contributor

rsc commented May 21, 2018

SGTM. The exit codes may not be needed. The point of the current "Program exited." is that the output is streaming so that line just says "it's done". The exact exit status is not usually terribly interesting.

@rsc rsc changed the title proposal: x/playground: streamline messages in output x/playground: make outcome of "Run" clearer May 21, 2018
@jimmyfrasche
Copy link
Member

My vote: only print non-zero exit status and only when not running tests.

Most of the time it's uninteresting but if you do os.Exit(3) that's output from the program that's just discarded now.

@ysmolski
Copy link
Member Author

@rsc, thanks, I have fixed examples regarding exit status.

@jimmyfrasche I will look into that. I have started sketching it and passing exit status might be too tricky to implement without breaking interfaces. Anyway, I will keep that in mind because I like the idea to show "Program exited with the code 3." in case of non zero exit code.

@ysmolski ysmolski self-assigned this Oct 8, 2018
@ysmolski
Copy link
Member Author

Please see the images and let me know what you think.

screen shot 2018-10-10 at 19 12 01
screen shot 2018-10-10 at 19 18 35
screen shot 2018-10-10 at 19 15 32
screen shot 2018-10-10 at 19 14 00
screen shot 2018-10-10 at 19 13 44
screen shot 2018-10-10 at 19 12 39
screen shot 2018-10-10 at 19 12 32
screen shot 2018-10-10 at 19 10 44

@ysmolski ysmolski changed the title x/playground: make outcome of "Run" clearer x/playground: make the outcome of "Run" clearer Oct 10, 2018
@ysmolski
Copy link
Member Author

ping @andybons @dmitshur @bradfitz @bcmills for the opinion :-)

@ysmolski
Copy link
Member Author

One format I am not sure about is the messages when program has exited with non-zero status. What do you think is the best?

  1. Program exited: exit status is 3.

  2. Program exited: status is 3.

  3. Program exited: code is 3.

I am inclined toward the 3rd option.

@gopherbot
Copy link

Change https://golang.org/cl/141477 mentions this issue: playground.js: make output of "Run" clearer

@gopherbot
Copy link

Change https://golang.org/cl/141478 mentions this issue: playground: make outcome of "Run" clearer

@jimmyfrasche
Copy link
Member

The precedence is that go run reports exit status 3

@ysmolski
Copy link
Member Author

Ok good. I somehow missed that new stuff. It was decided already, I will use it.

gopherbot pushed a commit to golang/tools that referenced this issue Oct 16, 2018
@ysmolski
playground.js: make output of "Run" clearer
321fe74 
This CL changes the last line displayed after the program was run
to display more details on what happened.

If the program cannot be built,
the last message is "Go build failed".

If the program has tests,
the last message is "All tests passed" in case of success.
Otherwise it is "N tests failed".

If the program has exited with non-zero code,
the exit message is postfixed with the code.

This CL adds output for timed out programs.

This CL is prerequisite for the backend change in CL 141478.
Dockerfile in playground repo has to be updated to include this CL.

Updates golang/go#10590
Updates golang/go#25454

Change-Id: Ie0a51b0729c574d2508a4a1b89f629def1d79fd6
Reviewed-on: https://go-review.googlesource.com/c/141477
Reviewed-by: Andrew Bonventre <andybons@golang.org>
gopherbot pushed a commit to golang/playground that referenced this issue Oct 16, 2018
@ysmolski
playground: make outcome of "Run" clearer
e03e829 
This CL changes the last line displayed after the program was run
to display more detail on what happened.

For more details see CL 141477.

Dockerfile in playground repo needs to be updated
to include x/tools with CL 141477.

Updates golang/go#10590
Updates golang/go#25454

Change-Id: I6bdef66e70d693540e4e7d30478ae9f0bf85d1ba
Reviewed-on: https://go-review.googlesource.com/c/141478
Reviewed-by: Andrew Bonventre <andybons@golang.org>
Run-TryBot: Andrew Bonventre <andybons@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/142637 mentions this issue: Dockerfile: include updated x/tools containing CL 141477

gopherbot pushed a commit to golang/playground that referenced this issue Oct 16, 2018
@ysmolski
Dockerfile: include updated x/tools containing CL 141477
ed387e6 
Updates golang/go#10590
Updates golang/go#25454

Change-Id: I807608979062f57268bf2b80edbd37d88e74b8f7
Reviewed-on: https://go-review.googlesource.com/c/142637
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@ysmolski
Copy link
Member Author

@dmitshur, this ticket can be closed after the deployment.

@gopherbot
Copy link

Change https://golang.org/cl/144078 mentions this issue: playground: fix the test broken by CL 141478

gopherbot pushed a commit to golang/playground that referenced this issue Oct 23, 2018
@ysmolski @katiehockman
playground: fix the test broken by CL 141478
ad3cdb9 
New fields were added to the response struct in CL 141478.
That was not accounted in the TestCommandHandler test.
This CL fixes the test by adding required fields.

Updates golang/go#25454

Change-Id: I374cef6199235d781bd83a60156ca4f0d90898ee
Reviewed-on: https://go-review.googlesource.com/c/144078
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Yury Smolsky <yury@smolsky.by>
Reviewed-by: Katie Hockman <katie@golang.org>
@dmitshur
Copy link
Contributor

@ysmolsky It's deployed now. Thank you!

@golang golang locked and limited conversation to collaborators Oct 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge Proposal Proposal-Accepted
Projects
None yet
Milestone
Proposal
Development

No branches or pull requests
5 participants
@rsc @ysmolski @jimmyfrasche @dmitshur @gopherbot