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

Add a GitHub workflow for continuous testing #167

Merged
merged 12 commits into from
Oct 21, 2020
Merged

Conversation

dscho
Copy link
Contributor

@dscho dscho commented Oct 21, 2020

The idea is to fix the tests so that they pass on Windows, and then have a GitHub workflow that runs on every push and Pull Request to ensure that no regressions are introduced on the main three platforms: Windows, macOS and Linux.

@dscho dscho force-pushed the meaow branch 3 times, most recently from c08dca3 to c709f81 Compare October 21, 2020 17:44
@dscho
Copy link
Contributor Author

dscho commented Oct 21, 2020

@newren I plan on working on this a little bit more. Please note that this PR takes the unrefined work I presented in https://lore.kernel.org/git/nycvar.QRO.7.76.6.2010211630010.56@tvgsbejvaqbjf.bet/ and refines it a little more.

@dscho
Copy link
Contributor Author

dscho commented Oct 21, 2020

TODO: use https://github.com/marketplace/actions/setup-python instead of downloading the full-fledged Git for Windows SDK (which takes ~3m20, quite a bit longer than actually running the tests).

[EDIT]: done. It's not without complications: setup-python does not install python3 into the PATH, but python.

@dscho dscho force-pushed the meaow branch 10 times, most recently from d0d2d7c to de0d28d Compare October 21, 2020 20:22
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
@dscho
Copy link
Contributor Author

dscho commented Oct 21, 2020

@newren while I have your attention... t9390.43 seems to fail for me all the time, even in WSL (i.e. Linux):

[...]
++ test_line_count = 5 ../filenames
++ test 3 '!=' 3
+++ wc -l
++ test 6 = 5
++ echo 'test_line_count: line count for ../filenames != 5'
test_line_count: line count for ../filenames != 5
++ cat ../filenames

numbers/medium.num
numbers/small.num
sequence/know
whatever
words/know
++ return 1
error: last command exited with $?=1
not ok 43 - --strip-blobs-with-ids

Is this test passing for you?

@dscho
Copy link
Contributor Author

dscho commented Oct 21, 2020

Is this test passing for you?

Oh wait, it passes for me on Linux with v2.29.0. Let me investigate.

@dscho
Copy link
Contributor Author

dscho commented Oct 21, 2020

Ahhh! 2ca6e36 is the "culprit" ;-)

@newren
Copy link
Owner

newren commented Oct 21, 2020

Ah, that '&&' was masking a bug in the testcase. Not sure how it got in there, but digging through it, the correct count is 6. The failure is in part due to the file named 'mercurial'; it was introduced in an "evil" merge and didn't exist in either parent. But my method for checking which files existed was kinda lame.

In addition to adding the '&&' and changing the expected count to 6, I should probably add a

		test_must_fail git rev-parse HEAD~4:mercurial &&

line to the test to ensure the mercurial file was ripped out.

@dscho
Copy link
Contributor Author

dscho commented Oct 21, 2020

Right. I actually managed to fix that, and pinpoint it to the commit that introduced the test case: b6a35f8

@dscho dscho force-pushed the meaow branch 2 times, most recently from dcb74d8 to ab84e7f Compare October 21, 2020 22:17
dscho added 7 commits October 22, 2020 00:29
The tests will otherwise fail.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
In that test case, we expect the line count to be 5, but it is actually
6 lines that we should expect:

	numbers/medium.num
	numbers/small.num
	sequence/know
	whatever
	words/know

Note the empty line at the top: this list is generated via `git log
--format=%n`, and that `%n` stands for "newline", meaning that we _must_
expect an empty line.

This expectation seems to have been broken already in the commit that
added the test case: b6a35f8 (filter-repo: implement
--strip-blobs-with-ids, 2019-05-30). It was hidden for such a long time
by a broken &&-chain, which we will fix next.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
The problem with this is that on Windows, we use the MSYS2 Bash which
uses the POSIX emulation layer called "MSYS2 runtime" that pretends that
there _is_ something like the `/dev/fd/` namespace, and tells `git.exe`
about it, but `git.exe` does not use the POSIX emulation layer, and
hence has no idea what Bash is talking about.

Besides, we should avoid pipes, just as we do in the Git project.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
While it is true that `colrm` is available on macOS by default, and even
in Ubuntu (thanks to the `bsdmainutils` package), it is not available on
Windows.

Let's use `cut` instead.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
MSYS2 tries to be very helpful, and in most cases it even works, by
converting parameters passed from inside an MSYS2 Bash to a non-MSYS2
application (such as `git.exe`) if they look like Unix-style paths or
path lists.

Sometimes, however, this automatic path conversion is unhelpful, e.g.
when passing the parameter `foo:.` to Git, which MSYS2 will readily
convert to a Windows-style path list: `foo;bar` (i.e. using a semicolon
instead of a colon).

Happily, there is a way to avoid that: the `MSYS_NO_PATHCONV` variable.
Let's use it.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho added 4 commits October 22, 2020 00:29
On Windows, there is no absolute path `/fake/path`, but MSYS2 (which Git
for Windows uses e.g. for running Bash scripts) pretends that it exists.
This only works within MSYS2 applications, of course, so... when MSYS2
sees that we hand a parameter to a non-MSYS2 application in a shell
script, it helpfully converts it to the full path (prepending MSYS2's
pseudo root directory).

Let's work around that by using a Win32-compatible path to begin with:
`$(pwd)` produces that on Windows. On other platforms, it still works.

As a bonus, this safe-guards our test against a setup where `/fake/path`
_actually exists_. Stranger things have been seen in the wild, after
all.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
The test case t9391.12 specifically wants to test LF vs CR/LF line
ending issues, expecting `core.autoCRLF` to default to `false`. This is
true on Linux and macOS and pretty much everywhere else, except on
Windows.

Let's make sure that the test operates with the `core.autoCRLF` value it
assumes to operate under.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Not all setups have `dos2unix`. Most notably, the Ubuntu and macOS
agents of GitHub Actions don't.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
@dscho dscho marked this pull request as ready for review October 21, 2020 22:39
@dscho
Copy link
Contributor Author

dscho commented Oct 21, 2020

There you go.

@dscho
Copy link
Contributor Author

dscho commented Oct 21, 2020

@newren would you kindly review this thing? I know, you don't like GitHub's PR UI because it does not let you comment easily on commit messages. Hopefully there is not a lot you need to comment about, though.

@newren newren merged commit e9e0308 into newren:main Oct 21, 2020
@newren
Copy link
Owner

newren commented Oct 21, 2020

Sweet, thanks @dscho; this is awesome!

@newren
Copy link
Owner

newren commented Oct 21, 2020

...and thanks for writing such great commit messages, as always. I learned a couple things about MSYS2 and Windows; I think one of those might be relevant to one of the issues that was filed if I can find it...

@dscho dscho deleted the meaow branch October 21, 2020 23:43
newren added a commit that referenced this pull request Apr 1, 2021
Add a GitHub workflow for continuous testing

Signed-off-by: Elijah Newren <newren@gmail.com>
fmigneault pushed a commit to fmigneault/git-filter-repo that referenced this pull request Dec 10, 2021
Add a GitHub workflow for continuous testing

Signed-off-by: Elijah Newren <newren@gmail.com>
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.

2 participants