Conversation
jorio
left a comment
There was a problem hiding this comment.
This is great! Not too different from how I'd have implemented it myself.
We just need some unit tests! You can use testFetchRemote as a starting point.
Feel free to "cheat" on the delays so the unit tests don't actually take 5 minutes to complete; I do this all the time.
(Don't worry too much about flaky Qt 5 tests in the CI for now)
(Notes for later - I don't mind merging a minimal version first, and ironing out the finer details later)Additional thoughts on this feature in increasing order of difficulty:
These are all outside the scope of this specific PR. I don't mind merging the current PR as it is first (as long as we turn off auto-fetch by default and we have a unit test). |
|
I tried to address the first bullet point here in 3bfd8b8, but I'm not sure how to write a unit test. An LLM came up with this, which seems like vaguely the right idea, but the def testTaskTerminationTerminatesProcess(tempDir, mainWindow):
"""Test that terminating a task also terminates its associated process."""
from gitfourchette import settings
delayCmd = ["python3", getTestDataPath("delay-cmd.py"), "--delay", "3", "--", settings.prefs.gitPath]
mainWindow.onAcceptPrefsDialog({"gitPath": shlex.join(delayCmd)})
wd = unpackRepo(tempDir)
barePath = makeBareCopy(wd, addAsRemote="localfs", preFetch=True)
rw = mainWindow.openRepo(wd)
# Start a fetch task that will take 3 seconds due to the delay command
from gitfourchette.tasks.nettasks import FetchRemotes
FetchRemotes.invoke(rw)
waitUntilTrue(lambda: rw.taskRunner.isBusy())
QTest.qWait(200)
assert rw.taskRunner.currentTask is not None
rw.taskRunner.killCurrentTask()
waitUntilTrue(lambda: not rw.taskRunner.isBusy())
assert rw.taskRunner.currentTask is None |
By default, all tasks run synchronously in unit tests. This facilitates writing test scenarios - no need to explicitly wait for each task to finish. However, the real application runs tasks asynchronously so that the user can still interact with the UI while a task is running. In this case, we want to interrupt a task while it's running, so we need async tasks. You can use the def testOngoingAutoFetchDoesntBlockOtherTasks(tempDir, mainWindow, taskThread):
from gitfourchette import settings
gitCmd = settings.prefs.gitPath
delayGitCmd = shlex.join(["python3", getTestDataPath("delay-cmd.py"), "--", settings.prefs.gitPath])
# Enable auto-fetch and make sure it'll keep RepoTaskRunner busy for a few seconds
mainWindow.onAcceptPrefsDialog({
"autoFetch": True,
"autoFetchMinutes": 1,
"gitPath": delayGitCmd,
})
wd = unpackRepo(tempDir)
barePath = makeBareCopy(wd, addAsRemote="localfs", preFetch=True)
with RepoContext(barePath) as bareRepo:
bareRepo.create_branch_on_head("new-remote-branch")
# Open the repo and wait for it to settle
mainWindow.openRepo(wd)
rw = waitForRepoWidget(mainWindow)
# Manually trigger the auto-fetch timer timeout to simulate the timer firing
rw.lastAutoFetchTime = 0
rw.onAutoFetchTimerTimeout()
# Make sure we're auto-fetching right now
assert isinstance(rw.taskRunner.currentTask, AutoFetchRemotes)
# Don't delay git for the next task
mainWindow.onAcceptPrefsDialog({"gitPath": gitCmd})
assert isinstance(rw.taskRunner.currentTask, AutoFetchRemotes) # just making sure a future version of onAcceptPrefsDialog doesn't kill the task...
# Perform a task - any task! - while auto-fetching is in progress.
# It shouldn't be blocked by an ongoing auto-fetch.
triggerMenuAction(mainWindow.menuBar(), "repo/new local branch")
newBranchDialog = waitForQDialog(rw, "new branch", t=NewBranchDialog)
newBranchDialog.ui.nameEdit.setText("not-blocked-by-auto-fetch")
newBranchDialog.accept()
waitUntilTrue(lambda: not rw.taskRunner.isBusy())
assert "not-blocked-by-auto-fetch" in rw.repo.branches.local
assert "localfs/new-remote-branch" not in rw.repo.branches.remote |
Co-authored-by: Iliyas Jorio <iliyas@jor.io>
Co-authored-by: Iliyas Jorio <iliyas@jor.io>
Co-authored-by: Iliyas Jorio <iliyas@jor.io>
|
Thank you for another PR! |
By default, auto-fetch every 5 minutes. Closes #56, except there's no per-remote setting; it just fetches all remotes.
I don't know if this is quite the right way to do this, comments are welcome :)