run integration tests with various git versions #2459

Ryooooooga · 2023-02-20T11:23:49Z

PR Description

#2456

although not in Docker

Please check if the PR fulfills these requirements

Cheatsheets are up-to-date (run go run scripts/cheatsheet/main.go generate)
Code has been formatted (see here)
Tests have been added/updated (see here for the integration test guide)
Text is internationalised (see here)
Docs (specifically docs/Config.md) have been updated if necessary
You've read through your own file changes for silly mistakes etc

github-actions · 2023-02-20T11:27:33Z

Uffizzi Ephemeral Environment `deployment-16733`

☁️ https://app.uffizzi.com/github.com/jesseduffield/lazygit/pull/2459

📄 View Application Logs etc.

What is Uffizzi? Learn more!

.github/workflows/ci.yml

Ryooooooga · 2023-02-22T12:39:40Z

pkg/integration/tests/sync/pull_rebase.go

@@ -22,7 +22,8 @@ var PullRebase = NewIntegrationTest(NewIntegrationTestArgs{
 		shell.SetBranchUpstream("master", "origin/master")

 		shell.HardReset("HEAD^^")
-		shell.EmptyCommit("four")


in older Git (2.??), git pull --rebase doesn't keep empty commits.

This begs the question of what we should do when we have a test that fails only on a single git version, or a subset of our supported versions. We really have three options:

update the test to be less picky and pass across all git versions

update the code to ensure functionality is the same across all git versions

capture the different functionality in the test

When it comes to something like git pull --rebase, it doesn't bother me that lazygit behaves differently depending on the git version, because:

users on an old git version won't be surprised if lazygit does what git already does on the command line

it would be a pain in the ass to add bespoke code getting the old git versions to behave like new git versions

So for that specific example, I'm fine with option 1 above.

And for the '--no-show-signature' example, option 2 works because it's trivial to do.

But I'd like to have a plan for when we do need to invoke option 3 i.e. we come across functionality that we really do want to test, where older versions behave differently or not at all (e.g. we just show a 'this feature is not supported on your git version, please update' alert panel).

For this, a couple options come to mind:
a) test-level separations: specify in the test which git versions the test is expected to pass on (and create a separate test for the other versions)
b) assertion-level separations: within the test, do something like:

if t.GitVersion("> 2.20") { // assert foo else { // assert bar }

I can imagine both test-level separations and assertion-level separations being needed at some point, but I imagine that assertion-level separations will be more common.

With all that said, what are your thoughts on adding some way to check the git version from within a test, so that we can capture differences across versions?

It would be useful to keep differences in behavior between Git versions in the test code.
It allows implementers to pay more attention to Git's behavior, and it may make it easier to determine the cause of test failures.

a) could be achieved with NewIntegrationTestArgs.Skip.

As an experiment, I tried to make the integration test move_to_earlier_commit.go pass with any git version (it fails because of --empty=keep). It would be possible, but require a bit of work, and I'm not sure it's worth it. On the other hand, not having the test to prove that the feature works at all in earlier git versions would also be a shame. What do you think?

A rough version of that experiment is here: stefanhaller@2878ef2. Not really pretty...

It would be great to make some progress with this. What do you both think about the experiment above?

For an upcoming PR I am in the situation again that I need version-specific integration tests. In this case I need to do a git rebase -i --update-refs, which is only available from 2.38 on. I don't see any other way than to disable that test for earlier versions. Not sure how to do that, the Skip property seems to accept only literal bools right now. Should we add a new MinGitVersion property to NewIntegrationTestArgs?

@stefanhaller How about changing Skip to func(...) bool?

I reckon we add a new property to NewIntegrationTestArgs as @stefanhaller suggests. I'm thinking the field would be GitVersion where the value would be a GitVersionRestriction struct instance that can be constructed with a method e.g. GreaterThan("2.1.1") or LessThan("2.1.1"), or Between("2.1.1", "2.1.4") or Include("2.4.1", "2.4.2", "2.4.3") or Exclude(...). That should give us the flexibility we want. What do we think about that?

We've got existing code in pkg/commands/git_commands/version.go we can make use of for this

I added something like this in the PR where I needed it, see 01d76d2. I only implemented GreaterThan, LessThan, and Include for now; we can add others when we need them, but I kind of doubt we will.

Actually I chose slightly different names, as I think in practice you'll want GreaterThanOrEqual in most cases. I found that a bit unwieldy, so I chose From and Before instead. I'm a bit unhappy about global functions with such generic names, but it does look good at the call site. 😄

jesseduffield · 2023-03-01T08:13:24Z

Nice work @Ryooooooga . My main hesitation here is that if a given git version fails, it might not be obvious why, and it would be good to fix it locally. If we were to use pre-built docker containers on CI, we could use the same image to be guaranteed to reproduce the issue locally. If we don't use docker, then we'll still need some way to go about reproducing locally (we could avoid docker on CI but then have some dockerfiles in the repo for local use, for example).

What are your thoughts?

Ryooooooga · 2023-03-01T13:39:12Z

@jesseduffield I created a Docker image for local integration_test: Ryooooooga#4

jesseduffield · 2023-03-18T07:46:48Z

.github/workflows/ci.yml

+          curl -sL "https://mirrors.edge.kernel.org/pub/software/scm/git/git-${{matrix.git-version}}.tar.xz" -o - | tar xJ -C "$HOME"
+          && cd "$HOME/git-${{matrix.git-version}}"
+          && ./configure
+          && make -j8


what does the -j8 refer to here?

I forgot that the -m option is optional, although it specifies the number of builds in parallel.

jesseduffield · 2023-03-18T07:47:34Z

.github/workflows/ci.yml

+          && ./configure
+          && make -j8
+      - name: Install Git ${{matrix.git-version}}
+        run: sudo make -C "$HOME/git-${{matrix.git-version}}" -j8 install


should we also restrict this step to only run if there was a cache miss?

Since /usr/local/ is not cached, make install must be run with or without cache.

This step takes very little time.

jesseduffield

Sorry it took me so long to review this :) Looking good

stefanhaller · 2023-03-18T10:14:35Z

The test apply_in_reverse_with_conflict.go fails in git versions 2.30.8 and earlier. Apparently the output Applied patch to 'file2' cleanly was only added more recently. Since it is not essential that we check this output, this patch should fix it:

diff --git a/pkg/integration/tests/patch_building/apply_in_reverse_with_conflict.go b/pkg/integration/tests/patch_building/apply_in_reverse_with_conflict.go
index 04d160a0..ebb84fbf 100644
--- a/pkg/integration/tests/patch_building/apply_in_reverse_with_conflict.go
+++ b/pkg/integration/tests/patch_building/apply_in_reverse_with_conflict.go
@@ -51,8 +51,7 @@ var ApplyInReverseWithConflict = NewIntegrationTest(NewIntegrationTestArgs{
 
 		t.ExpectPopup().Alert().
 			Title(Equals("Error")).
-			Content(Contains("Applied patch to 'file1' with conflicts.").
-				Contains("Applied patch to 'file2' cleanly.")).
+			Content(Contains("Applied patch to 'file1' with conflicts.")).
 			Confirm()
 
 		t.Views().Files().

@stefanhaller

By: @stefanhaller Ref: jesseduffield#2459 (comment)

@stefanhaller

By: @stefanhaller Ref: jesseduffield#2459 (comment)

@stefanhaller

By: @stefanhaller Ref: jesseduffield#2459 (comment)

@stefanhaller

By: @stefanhaller Ref: jesseduffield#2459 (comment)

stefanhaller · 2023-06-27T08:24:47Z

@Ryooooooga Do you have time to pick this up again?

We found another regression with an older git version (#2745), and this PR would have caught it. It would be so cool to have this on master.

Let me know if you don't have time, I would consider taking it on in that case.

stefanhaller · 2023-07-02T20:12:54Z

Since I didn't hear back, I went ahead and made my own version of this: #2754. It uses a very similar approach as this PR, but it's also different enough that it's worth re-reviewing thoroughly.

jesseduffield · 2023-07-10T12:31:48Z

#2754 is approved so I'll close this off. But thanks for laying the groundwork @Ryooooooga !

@Ryooooooga

Run integration tests with various different git versions, more or less randomly picked from our range of supported versions. Based on @Ryooooooga's work in #2459, but also restructured a bit. All tests pass for all git versions, but only after cherry-picking #2747. I decided to go with @Ryooooooga's approach and do it without using docker. I also didn't use docker locally; to reproduce the various failures that I had to fix, I simply installed the respective git versions locally and used something like `PATH=~/git-versions/2.25.1/bin:$PATH ./scripts/run_integration_tests.sh`.

Ryooooooga commented Feb 20, 2023

View reviewed changes

.github/workflows/ci.yml Outdated Show resolved Hide resolved

Ryooooooga force-pushed the ci-git-version branch 10 times, most recently from ccb4444 to b2dfb7b Compare February 22, 2023 12:37

Ryooooooga commented Feb 22, 2023

View reviewed changes

Ryooooooga force-pushed the ci-git-version branch 7 times, most recently from f8ea033 to eea16a7 Compare February 27, 2023 06:25

Ryooooooga force-pushed the ci-git-version branch 2 times, most recently from 85231b9 to ada5014 Compare March 1, 2023 11:34

Ryooooooga force-pushed the ci-git-version branch from ada5014 to 128e3f1 Compare March 10, 2023 15:46

jesseduffield reviewed Mar 18, 2023

View reviewed changes

Ryooooooga force-pushed the ci-git-version branch from 128e3f1 to dfed279 Compare March 22, 2023 14:02

Ryooooooga force-pushed the ci-git-version branch from dfed279 to a761f75 Compare March 22, 2023 14:04

Ryooooooga added a commit to Ryooooooga/lazygit that referenced this pull request Mar 22, 2023

test: fix conflict test

c90d59b

By: @stefanhaller Ref: jesseduffield#2459 (comment)

Ryooooooga force-pushed the ci-git-version branch from a761f75 to c90d59b Compare March 22, 2023 14:07

Ryooooooga added a commit to Ryooooooga/lazygit that referenced this pull request Mar 22, 2023

test: fix conflict test

0bb767f

By: @stefanhaller Ref: jesseduffield#2459 (comment)

Ryooooooga force-pushed the ci-git-version branch from c90d59b to 0bb767f Compare March 22, 2023 14:11

Ryooooooga added a commit to Ryooooooga/lazygit that referenced this pull request Mar 22, 2023

test: fix conflict test

96b68eb

By: @stefanhaller Ref: jesseduffield#2459 (comment)

Ryooooooga force-pushed the ci-git-version branch 2 times, most recently from e711ea3 to 28e8e78 Compare March 22, 2023 14:41

Ryooooooga added 10 commits April 17, 2023 19:11

ci: run integration tests with various git versions

8c14890

chore(git_commands): use git log --no-show-signature

f0770d6

fix: fix stash commands to support old git

260f543

test: fix rebase test

7eef14d

test: fix conflict test

f9dda1b

By: @stefanhaller Ref: jesseduffield#2459 (comment)

ci: fix ci

1aa61b6

ci: update actions/checkout to v3

6ecf711

ci: install libcurl4-gnutls-dev

63ea025

ci: fix order of actions/checkout

ced481a

chore(git_commands): support old git version (git rebase --empty=keep)

5f1786d

Ryooooooga force-pushed the ci-git-version branch from 318c416 to 5f1786d Compare April 17, 2023 10:13

stefanhaller mentioned this pull request Jul 2, 2023

Run integration tests with all supported git versions #2754

Merged

jesseduffield closed this Jul 10, 2023

Uh oh!

run integration tests with various git versions #2459

run integration tests with various git versions #2459

Uh oh!

Conversation

Ryooooooga commented Feb 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Feb 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uffizzi Ephemeral Environment deployment-16733

Uh oh!

Uh oh!

Ryooooooga Feb 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jesseduffield Apr 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jesseduffield commented Mar 1, 2023

Uh oh!

Ryooooooga commented Mar 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jesseduffield left a comment

Choose a reason for hiding this comment

Uh oh!

stefanhaller commented Mar 18, 2023

Uh oh!

stefanhaller commented Jun 27, 2023

Uh oh!

stefanhaller commented Jul 2, 2023

Uh oh!

jesseduffield commented Jul 10, 2023

Uh oh!

Uh oh!

Ryooooooga commented Feb 20, 2023 •

edited

Loading

github-actions bot commented Feb 20, 2023 •

edited

Loading

Uffizzi Ephemeral Environment `deployment-16733`

Ryooooooga Feb 22, 2023 •

edited

Loading

jesseduffield Apr 13, 2023 •

edited

Loading

Ryooooooga commented Mar 1, 2023 •

edited

Loading