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

Track busy/idle state for integration tests #2765

Merged
merged 19 commits into from
Jul 10, 2023
Merged

Commits on Jul 8, 2023

  1. Bump gocui

    This includes new gocui logic for tracking busy/idle program state
    jesseduffield committed Jul 8, 2023
    Configuration menu
    Copy the full SHA
    631cf1e View commit details
    Browse the repository at this point in the history
  2. Add busy count for integration tests

    Integration tests need to be notified when Lazygit is idle so they can progress to the next assertion / user action.
    jesseduffield committed Jul 8, 2023
    Configuration menu
    Copy the full SHA
    6c4e7ee View commit details
    Browse the repository at this point in the history
  3. Handle pending actions properly in git commands that require credentials

    I don't know if this is a hack or not: we run a git command and increment the pending action
    count to 1 but at some point the command requests a username or password, so we need to prompt
    the user to enter that. At that point we don't want to say that there is a pending action,
    so we decrement the action count before prompting the user and then re-increment it again afterward.
    
    Given that we panic when the counter goes below zero, it's important that it's not zero
    when we run the git command (should be impossible anyway).
    
    I toyed with a different approach using channels and a long-running goroutine that
    handles all commands that request credentials but it feels over-engineered compared to this
    commit's approach.
    jesseduffield committed Jul 8, 2023
    Configuration menu
    Copy the full SHA
    26ca41a View commit details
    Browse the repository at this point in the history
  4. Remove redundant waitgroup

    Turns out we're just running our refresh functions one after the other which isn't ideal but we can fix that separately.
    As it stands this wait group isn't doing anything.
    jesseduffield committed Jul 8, 2023
    Configuration menu
    Copy the full SHA
    015a04f View commit details
    Browse the repository at this point in the history
  5. Wait for intro before doing any of our refresh functions

    We were doing this already for fetching but not for refreshing files so I'm making it consistent.
    jesseduffield committed Jul 8, 2023
    Configuration menu
    Copy the full SHA
    b19943a View commit details
    Browse the repository at this point in the history

Commits on Jul 9, 2023

  1. Remove retry logic in integration tests

    I want to see how we go removing all retry logic within a test. Lazygit should be trusted to tell us when it's no longer busy,
    and if it that proves false we should fix the issue in the code rather than being lenient in the tests
    jesseduffield committed Jul 9, 2023
    Configuration menu
    Copy the full SHA
    c7a3b69 View commit details
    Browse the repository at this point in the history
  2. Add mutex for refreshing branches

    We had a race condition due to refreshing branches in two different places, one which refreshed reflog commits
    beforehand. The race condition meant that upon load we wouldn't see recency values (provided by the reflog commits)
    against the branches
    jesseduffield committed Jul 9, 2023
    Configuration menu
    Copy the full SHA
    e588355 View commit details
    Browse the repository at this point in the history
  3. Only attempt integration tests once

    I was able to get all integration tests passing 20 times in a row without any retries so I'm going to see
    if we can rely on that in CI
    jesseduffield committed Jul 9, 2023
    Configuration menu
    Copy the full SHA
    6282d55 View commit details
    Browse the repository at this point in the history
  4. Fix race condition

    We had some test flakiness involving the index.lock file which is fixed by this commit.
    We shouldn't be accessing newTaskID without the mutex, although I'm surprised that this
    actually fixes the issue. Surely we don't have tasks (which typically render to the main
    view) which use index.lock?
    jesseduffield committed Jul 9, 2023
    Configuration menu
    Copy the full SHA
    bf7726d View commit details
    Browse the repository at this point in the history
  5. Fix test

    It's still skipped but it had an error
    jesseduffield committed Jul 9, 2023
    Configuration menu
    Copy the full SHA
    fdee0e1 View commit details
    Browse the repository at this point in the history
  6. Configuration menu
    Copy the full SHA
    9e79ee5 View commit details
    Browse the repository at this point in the history
  7. Use first class task objects instead of global counter

    The global counter approach is easy to understand but it's brittle and depends on implicit behaviour that is not very discoverable.
    
    With a global counter, if any goroutine accidentally decrements the counter twice, we'll think lazygit is idle when it's actually busy.
    Likewise if a goroutine accidentally increments the counter twice we'll think lazygit is busy when it's actually idle.
    With the new approach we have a map of tasks where each task can either be busy or not. We create a new task and add it to the map
    when we spawn a worker goroutine (among other things) and we remove it once the task is done.
    
    The task can also be paused and continued for situations where we switch back and forth between running a program and asking for user
    input.
    
    In order for this to work with `git push` (and other commands that require credentials) we need to obtain the task from gocui when
    we create the worker goroutine, and then pass it along to the commands package to pause/continue the task as required. This is
    MUCH more discoverable than the old approach which just decremented and incremented the global counter from within the commands package,
    but it's at the cost of expanding some function signatures (arguably a good thing).
    
    Likewise, whenever you want to call WithWaitingStatus or WithLoaderPanel the callback will now have access to the task for pausing/
    continuing. We only need to actually make use of this functionality in a couple of places so it's a high price to pay, but I don't
    know if I want to introduce a WithWaitingStatusTask and WithLoaderPanelTask function (open to suggestions).
    jesseduffield committed Jul 9, 2023
    Configuration menu
    Copy the full SHA
    14ecc15 View commit details
    Browse the repository at this point in the history
  8. Use mutex on cached git config

    This fixes a race condition caused by a concurrent map read and write
    jesseduffield committed Jul 9, 2023
    Configuration menu
    Copy the full SHA
    8964ced View commit details
    Browse the repository at this point in the history

Commits on Jul 10, 2023

  1. Use an interface for tasks instead of a concrete struct

    By using an interface for tasks we can use a fake implementation in tests with extra methods
    jesseduffield committed Jul 10, 2023
    Configuration menu
    Copy the full SHA
    6b93904 View commit details
    Browse the repository at this point in the history
  2. Refactor

    jesseduffield committed Jul 10, 2023
    Configuration menu
    Copy the full SHA
    a0154dc View commit details
    Browse the repository at this point in the history
  3. Fix flakey misc/initial_open test

    I've simplifiied the code because it was too complex for the current requirements, and this fixed the misc/initial_open
    test which was occasionally failing due to a race condition around busy tasks
    jesseduffield committed Jul 10, 2023
    Configuration menu
    Copy the full SHA
    c05a1ae View commit details
    Browse the repository at this point in the history
  4. Fix flakey pull_merge_conflict test

    It's not clear what was happening but it seemed like we sometimes weren't
    fully writing to our stdout buffer (which is used for the error message)
    even though we had returned from cmd.Wait().
    
    Not sure what the cause was but removing an unnecessary goroutine fixed it.
    jesseduffield committed Jul 10, 2023
    Configuration menu
    Copy the full SHA
    9061305 View commit details
    Browse the repository at this point in the history
  5. Configuration menu
    Copy the full SHA
    d44d164 View commit details
    Browse the repository at this point in the history
  6. Retry on index.lock error

    I don't know why we're getting index.lock errors but they're impossile to stop
    anyway given that other processes can be calling git commands. So we're retrying
    a few times before re-raising. To do this we need to clone the command and the current
    implementation for that is best-effort.
    
    I do worry about the maintainability of that but we'll see how it goes.
    
    Also, I thought you'd need to clone the task (if it exists) but now I think not;
    as long as you don't call done twice on it you should be fine, and you shouldn't
    be done'ing a task as part of running a command: that should happen higher up.
    jesseduffield committed Jul 10, 2023
    Configuration menu
    Copy the full SHA
    16ed3c2 View commit details
    Browse the repository at this point in the history