Skip to content

Conversation

@daramayis
Copy link

@daramayis daramayis commented Nov 11, 2022

PR Description

This PR is a part of #2234 and helps demonstrate Uffizzi integration with lazygit. This will help create preview environments on all of lazygit Pull Requests and help contributors including the maintainers iterate faster on their PRs.
I have created a PR over daramayis#1 (comment) to show what it looks like to have a preview environment deployed against your PR.

You can check out the preview over here as well.

@waveywaves

@jesseduffield
Copy link
Owner

Thanks @daramayis I'm interested in seeing how useful this could be for us. One thing is that opening lazygit on its own isn't particularly useful: typically a PR is trying to address some problem, and the expectation is that the PR adds one or more integration tests to verify the change. So I'd like to see go run cmd/integration_test/main.go tui be invoked instead of lazygit, which brings up a terminal UI that allows you to select integration tests to run (either directly or in a sandbox mode where you can click around yourself).

Currently that program will compile lazygit before running it for a given integration test. If that proves too slow for our test box we could instead pre-compile lazygit and pass an env var or argument to the tui program to use the pre-compiled lazygit, so that the docker file can compile lazygit once without us having to do it within the running container.

On a separate note, how would one typically go to the link? Would a bot post a comment on the PR with a link to the new session every time a commit is pushed?

@jesseduffield
Copy link
Owner

Also tagging @mark2185 and @Ryooooooga for their thoughts. Do you think this would prove useful?

Thinking out loud: one potential use case is for people who want to demonstrate some bug: they could create a PR containing an integration test which demonstrates the problem, and then a contributor could verify it without needing to check out the branch. Having said that, if somebody is willing to go and create an integration test for the sake of reproduction, they'd probably be willing to just fix the issue itself.

@mark2185
Copy link
Collaborator

mark2185 commented Nov 12, 2022

Having said that, if somebody is willing to go and create an integration test for the sake of reproduction, they'd probably be willing to just fix the issue itself.

Exactly.

I'd rather not put the burden on the reporter.

What it might come handy for is maybe presenting new features, e.g. "look at these new icons", or "could you try to reproduce your bug on this branch, to see if it's fixed" for people not willing or able to build a custom branch. But that often requires a custom setup, a private repo, or something else.

I dunno, I'm not quite sold on it (at least not yet), but maybe I'm not the target audience.

@daramayis
Copy link
Author

Hi @jesseduffield and @mark2185 ,
I have fixed and now we run the tests — go run cmd/integration_test/main.go tui.
Here you can check https://app.uffizzi.com//github.com/daramayis/f-lazygit/pull/1

@jesseduffield
Copy link
Owner

@daramayis testing that out I see visual artefacts:
image

I assume you need to bump your xterm library or something.

It's also still quite slow. Is it possible to speed it up?

@daramayis
Copy link
Author

Hi @jesseduffield

Thank you for the review.

The visual artifacts look fixed after the image components bump.
Regarding the slowness — I did some tests and found that sometimes it's stuck or slowed down due to tests.
I attached the stack trace of that unstable tests at the end of this comment.

Beside of that we also updated the lazygit preview environments resources amount — we gave 2GB of memory and removed the CPU limitation.

The preview link: https://app.uffizzi.com//github.com/daramayis/f-lazygit/pull/1


Stacktrace of random unstable tests

press enter to return
2023/01/11 11:07:43 path: /go/src/github.com/jesseduffield/lazygit/test/integration_new/cherry_pick/cherry_pick_conflicts
panic: Expected confirmation popup to be focused

goroutine 26 [running]:
github.com/jesseduffield/lazygit/pkg/gui.(*GuiDriver).Fail(0xee6b2800?, {0xc14162, 0x29})
  /go/src/github.com/jesseduffield/lazygit/pkg/gui/gui_driver.go:58 +0x54
github.com/jesseduffield/lazygit/pkg/integration/components.(*Assert).Fail(...)
  /go/src/github.com/jesseduffield/lazygit/pkg/integration/components/assert.go:238
github.com/jesseduffield/lazygit/pkg/integration/components.(*Assert).assertWithRetries(0xc000521a10, 0xc0005d4670)
  /go/src/github.com/jesseduffield/lazygit/pkg/integration/components/assert.go:233 +0xfb
github.com/jesseduffield/lazygit/pkg/integration/components.(*Assert).InConfirm(0xc0005d8000?)
  /go/src/github.com/jesseduffield/lazygit/pkg/integration/components/assert.go:152 +0x35
github.com/jesseduffield/lazygit/pkg/integration/tests/cherry_pick.glob..func6(_, _, _, {{{0xbd82a1, 0x1}, {0xbdab3a, 0x5}, {0xbdabbc, 0x5}, {0xcff548, ...}, ...}, ...})
  /go/src/github.com/jesseduffield/lazygit/pkg/integration/tests/cherry_pick/cherry_pick_conflicts.go:62 +0x46f
github.com/jesseduffield/lazygit/pkg/integration/components.(*IntegrationTest).Run(0xc0001015e0, {0xd0f408?, 0xc000010140})
  /go/src/github.com/jesseduffield/lazygit/pkg/integration/components/test.go:101 +0x231
github.com/jesseduffield/lazygit/pkg/gui.(*Gui).handleTestMode.func1()
  /go/src/github.com/jesseduffield/lazygit/pkg/gui/test_mode.go:24 +0x83
created by github.com/jesseduffield/lazygit/pkg/gui.(*Gui).handleTestMode
  /go/src/github.com/jesseduffield/lazygit/pkg/gui/test_mode.go:21 +0x98
2023/01/11 11:07:51 exit status 2



press enter to return
2023/01/11 11:08:32 path: /go/src/github.com/jesseduffield/lazygit/test/integration_new/branch/rebase
panic: Expected confirmation popup to be focused
goroutine 29 [running]:
github.com/jesseduffield/lazygit/pkg/gui.(*GuiDriver).Fail(0xee6b2800?, {0xc14162, 0x29})
  /go/src/github.com/jesseduffield/lazygit/pkg/gui/gui_driver.go:58 +0x54
github.com/jesseduffield/lazygit/pkg/integration/components.(*Assert).Fail(...)
  /go/src/github.com/jesseduffield/lazygit/pkg/integration/components/assert.go:238
github.com/jesseduffield/lazygit/pkg/integration/components.(*Assert).assertWithRetries(0xc0004b93b0, 0xc0002a46a0)
  /go/src/github.com/jesseduffield/lazygit/pkg/integration/components/assert.go:233 +0xfb
github.com/jesseduffield/lazygit/pkg/integration/components.(*Assert).InConfirm(0xc0004b93b0?)
  /go/src/github.com/jesseduffield/lazygit/pkg/integration/components/assert.go:152 +0x35
github.com/jesseduffield/lazygit/pkg/integration/tests/branch.glob..func6(_, _, _, {{{0xbd82a1, 0x1}, {0xbdab3a, 0x5}, {0xbdabbc, 0x5}, {0xcff548, ...}, ...}, ...})
  /go/src/github.com/jesseduffield/lazygit/pkg/integration/tests/branch/rebase.go:45 +0x28f
github.com/jesseduffield/lazygit/pkg/integration/components.(*IntegrationTest).Run(0xc0001014a0, {0xd0f408?, 0xc000128018})
  /go/src/github.com/jesseduffield/lazygit/pkg/integration/components/test.go:101 +0x231
github.com/jesseduffield/lazygit/pkg/gui.(*Gui).handleTestMode.func1()
  /go/src/github.com/jesseduffield/lazygit/pkg/gui/test_mode.go:24 +0x83
created by github.com/jesseduffield/lazygit/pkg/gui.(*Gui).handleTestMode
  /go/src/github.com/jesseduffield/lazygit/pkg/gui/test_mode.go:21 +0x98
2023/01/11 11:08:40 exit status 2

@jesseduffield
Copy link
Owner

That's looking good @daramayis . I want to trial this out for a month and then decide whether to keep it. I'll admit, at the moment I'm not confident we'll stick with it, but I also have a negativity bias so in order to compensate for that I'll give it a go.

One thing I want to know: how will this actually work? Would a bot post a comment with the link upon raising a PR? Would it post a comment upon each push? If it's the latter, that will get very noisy.

@daramayis
Copy link
Author

@jesseduffield

One thing I want to know: how will this actually work? Would a bot post a comment with the link upon raising a PR? Would it post a comment upon each push? If it's the latter, that will get very noisy.

On each push — it will just edit(update) the existing comment.

@jesseduffield
Copy link
Owner

Sweet, so is this PR good to merge?

@daramayis
Copy link
Author

@jesseduffield yes — it's ready to merge 🚀

@daramayis
Copy link
Author

Hey @jesseduffield,

Friendly ping — do you have a timeline for looking at this?

Thank you.

@jesseduffield jesseduffield merged commit 89a09be into jesseduffield:master Jan 26, 2023
@jesseduffield
Copy link
Owner

@daramayis it's now merged :) sorry for the wait

@daramayis
Copy link
Author

No problem, thank you.

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.

3 participants