| Message ID | f40f62f257de38c3a38e942eb9ca1d2109c2b059.1721740612.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Improvements for Perforce tests | expand |
On 24/07/23 04:05PM, Patrick Steinhardt wrote: > Some of the tests in t98xx modify the Perforce depot in ways that the > tool wouldn't normally allow. This is done to test behaviour of git-p4 > in certain edge cases that we have observed in the wild, but which > should in theory not be possible. If in theory these edge cases being tested should not be possible, that sounds like a bug and maybe in newer versions of p4 that is no longer relevant? Does it make sense to even support these rather intimate test cases going forward? Maybe we could instead skip these tests for newer versions? > Naturally, modifying the depot on disk directly is quite intimate with > the tool and thus prone to breakage when Perforce updates the way that > data is stored. And indeed, those tests are broken nowadays with r23 of > Perforce. While a file revision was previously stored as plain file > "depot/file,v", it is now stored in a directory "depot/file,d" with > compression. s/plain/a plain/ This sounds like a bit of a maintenance headache, especially if there are not many eyes on it to begin with. I guess this ties in with other discussion from this thread about whether and of git-p4 should remain in the codebase. > Adapt those tests to handle both old- and new-style depot layouts. > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > t/t9800-git-p4-basic.sh | 13 +++++++++++-- > t/t9802-git-p4-filetype.sh | 15 ++++++++++++--- > t/t9825-git-p4-handle-utf16-without-bom.sh | 18 +++++++++++++++--- > 3 files changed, 38 insertions(+), 8 deletions(-) > > diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh > index 53af8e34ac..4e95622670 100755 > --- a/t/t9800-git-p4-basic.sh > +++ b/t/t9800-git-p4-basic.sh > @@ -297,8 +297,17 @@ test_expect_success 'exit when p4 fails to produce marshaled output' ' > # p4 changes, files, or describe; just in p4 print. If P4CLIENT is unset, the > # message will include "Librarian checkout". > test_expect_success 'exit gracefully for p4 server errors' ' > - test_when_finished "mv \"$db\"/depot/file1,v,hidden \"$db\"/depot/file1,v" && > - mv "$db"/depot/file1,v "$db"/depot/file1,v,hidden && > + case "$(echo "$db"/depot/file1*)" in > + *,v) > + test_when_finished "mv \"$db\"/depot/file1,v,hidden \"$db\"/depot/file1,v" && > + mv "$db"/depot/file1,v "$db"/depot/file1,v,hidden;; > + *,d) > + path="$(echo "$db"/depot/file1,d/*.gz)" && > + test_when_finished "mv \"$path\",hidden \"$path\"" && > + mv "$path" "$path",hidden;; > + *) > + BUG "unhandled p4d layout";; > + esac && I'm not familiar with Perforce, but the test looks like it is simply appending ",hidden" to the file name. I assume this to trigger some error. We are simply extending the test to also perform the same rename if, instead of `depot/file1,f`, a newer version uses `depot/file1,d`. Makes sense to me, but without surrounding context its rather difficult to understand that the "case" statement here pertains to different Perforce versions that may be used. It might be nice to have a comment explaining this. > test_when_finished cleanup_git && > test_expect_code 1 git p4 clone --dest="$git" //depot@1 >out 2>err && > test_grep "Error from p4 print" err > diff --git a/t/t9802-git-p4-filetype.sh b/t/t9802-git-p4-filetype.sh > index bb236cd2b5..557e11b16c 100755 > --- a/t/t9802-git-p4-filetype.sh > +++ b/t/t9802-git-p4-filetype.sh > @@ -301,9 +301,18 @@ test_expect_success SYMLINKS 'empty symlink target' ' > # @@ > # > cd "$db/depot" && > - sed "/@target1/{; s/target1/@/; n; d; }" \ > - empty-symlink,v >empty-symlink,v.tmp && > - mv empty-symlink,v.tmp empty-symlink,v > + case "$(echo empty-symlink*)" in > + empty-symlink,v) > + sed "/@target1/{; s/target1/@/; n; d; }" \ > + empty-symlink,v >empty-symlink,v.tmp && > + mv empty-symlink,v.tmp empty-symlink,v;; > + empty-symlink,d) > + path="empty-symlink,d/$(ls empty-symlink,d/ | tail -n1)" && > + rm "$path" && > + gzip </dev/null >"$path";; > + *) > + BUG "unhandled p4d layout";; > + esac Looks like for this test, for previous Perforce versions we were making a symlink point to nothing. For newer versions of Perforce, we seem to accomplish the same think by creating an empty compressed file. Seems reasonable to me. > ) && > ( > # Make sure symlink really is empty. Asking > diff --git a/t/t9825-git-p4-handle-utf16-without-bom.sh b/t/t9825-git-p4-handle-utf16-without-bom.sh > index f049ff8229..8e34f72198 100755 > --- a/t/t9825-git-p4-handle-utf16-without-bom.sh > +++ b/t/t9825-git-p4-handle-utf16-without-bom.sh > @@ -22,9 +22,21 @@ test_expect_success 'init depot with UTF-16 encoded file and artificially remove > cd db && > p4d -jc && > # P4D automatically adds a BOM. Remove it here to make the file invalid. > - sed -e "\$d" depot/file1,v >depot/file1,v.new && > - mv depot/file1,v.new depot/file1,v && > - printf "@$UTF16@" >>depot/file1,v && > + case "$(echo depot/file1*)" in > + depot/file1,v) > + sed -e "\$d" depot/file1,v >depot/file1,v.new && > + mv depot/file1,v.new depot/file1,v && > + printf "@$UTF16@" >>depot/file1,v;; > + depot/file1,d) > + path="$(echo depot/file1,d/*.gz)" && > + gunzip -c "$path" >"$path.unzipped" && > + sed -e "\$d" "$path.unzipped" >"$path.new" && > + printf "$UTF16" >>"$path.new" && > + gzip -c "$path.new" >"$path" && > + rm "$path.unzipped" "$path.new";; > + *) > + BUG "unhandled p4d layout";; > + esac && Looks like the same deal here. For the new version we are modifying some file, but first have to uncompress it and then recompress it after it is modified. Again seems reasonable. Thanks -Justin > p4d -jrF checkpoint.1 > ) > ' > -- > 2.46.0.rc1.dirty >
On Tue, Jul 30, 2024 at 05:41:18PM -0500, Justin Tobler wrote: > On 24/07/23 04:05PM, Patrick Steinhardt wrote: > > Some of the tests in t98xx modify the Perforce depot in ways that the > > tool wouldn't normally allow. This is done to test behaviour of git-p4 > > in certain edge cases that we have observed in the wild, but which > > should in theory not be possible. > > If in theory these edge cases being tested should not be possible, that > sounds like a bug and maybe in newer versions of p4 that is no longer > relevant? Does it make sense to even support these rather intimate test > cases going forward? Maybe we could instead skip these tests for newer > versions? I wouldn't feel comfortable skipping this without knowing anything about the underlying bug that had been tested in the past. If we removed it, then we'd be second guessing whether or not that bug is still possible. Seemingly, it wasn't possible to trigger it without bugs in the past either, so it feels like we'd be assuming too much. > > Naturally, modifying the depot on disk directly is quite intimate with > > the tool and thus prone to breakage when Perforce updates the way that > > data is stored. And indeed, those tests are broken nowadays with r23 of > > Perforce. While a file revision was previously stored as plain file > > "depot/file,v", it is now stored in a directory "depot/file,d" with > > compression. > > s/plain/a plain/ > > This sounds like a bit of a maintenance headache, especially if there > are not many eyes on it to begin with. I guess this ties in with other > discussion from this thread about whether and of git-p4 should remain in > the codebase. Oh, it certainly isn't great. Whether it is a headache remains to be seen. If we had to pile onto hacks like this in the future then I'd also vote to remove or skip such tests. But we didn't really have to do a lot of maintenance for the Perforce tests in the past either, so I rather assume that it's going to be okayish in the future, too. Part of the reason is likely that git-p4 simply isn't well-maintained, and the less maintained something is the less churn you have. But for now I only see two ways out of it: - Either we get somebody willing to adopt it, and in that case it will be their responsibility to handle such potential headaches. In other words, I'll leave it to them to decide what to do with those tests, also because they are going to be better equipped with knowledge around Perforce itself. - Or we remove it, and then it doesn't matter much anyway. All to say that I'm merely being pragmatic and punt the issue to the future. > > Adapt those tests to handle both old- and new-style depot layouts. > > > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > > --- > > t/t9800-git-p4-basic.sh | 13 +++++++++++-- > > t/t9802-git-p4-filetype.sh | 15 ++++++++++++--- > > t/t9825-git-p4-handle-utf16-without-bom.sh | 18 +++++++++++++++--- > > 3 files changed, 38 insertions(+), 8 deletions(-) > > > > diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh > > index 53af8e34ac..4e95622670 100755 > > --- a/t/t9800-git-p4-basic.sh > > +++ b/t/t9800-git-p4-basic.sh > > @@ -297,8 +297,17 @@ test_expect_success 'exit when p4 fails to produce marshaled output' ' > > # p4 changes, files, or describe; just in p4 print. If P4CLIENT is unset, the > > # message will include "Librarian checkout". > > test_expect_success 'exit gracefully for p4 server errors' ' > > - test_when_finished "mv \"$db\"/depot/file1,v,hidden \"$db\"/depot/file1,v" && > > - mv "$db"/depot/file1,v "$db"/depot/file1,v,hidden && > > + case "$(echo "$db"/depot/file1*)" in > > + *,v) > > + test_when_finished "mv \"$db\"/depot/file1,v,hidden \"$db\"/depot/file1,v" && > > + mv "$db"/depot/file1,v "$db"/depot/file1,v,hidden;; > > + *,d) > > + path="$(echo "$db"/depot/file1,d/*.gz)" && > > + test_when_finished "mv \"$path\",hidden \"$path\"" && > > + mv "$path" "$path",hidden;; > > + *) > > + BUG "unhandled p4d layout";; > > + esac && > > I'm not familiar with Perforce, but the test looks like it is simply > appending ",hidden" to the file name. I assume this to trigger some > error. > > We are simply extending the test to also perform the same rename if, > instead of `depot/file1,f`, a newer version uses `depot/file1,d`. > > Makes sense to me, but without surrounding context its rather difficult > to understand that the "case" statement here pertains to different > Perforce versions that may be used. It might be nice to have a comment > explaining this. Fair, I will add a comment to all three sites. Patric
diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh index 53af8e34ac..4e95622670 100755 --- a/t/t9800-git-p4-basic.sh +++ b/t/t9800-git-p4-basic.sh @@ -297,8 +297,17 @@ test_expect_success 'exit when p4 fails to produce marshaled output' ' # p4 changes, files, or describe; just in p4 print. If P4CLIENT is unset, the # message will include "Librarian checkout". test_expect_success 'exit gracefully for p4 server errors' ' - test_when_finished "mv \"$db\"/depot/file1,v,hidden \"$db\"/depot/file1,v" && - mv "$db"/depot/file1,v "$db"/depot/file1,v,hidden && + case "$(echo "$db"/depot/file1*)" in + *,v) + test_when_finished "mv \"$db\"/depot/file1,v,hidden \"$db\"/depot/file1,v" && + mv "$db"/depot/file1,v "$db"/depot/file1,v,hidden;; + *,d) + path="$(echo "$db"/depot/file1,d/*.gz)" && + test_when_finished "mv \"$path\",hidden \"$path\"" && + mv "$path" "$path",hidden;; + *) + BUG "unhandled p4d layout";; + esac && test_when_finished cleanup_git && test_expect_code 1 git p4 clone --dest="$git" //depot@1 >out 2>err && test_grep "Error from p4 print" err diff --git a/t/t9802-git-p4-filetype.sh b/t/t9802-git-p4-filetype.sh index bb236cd2b5..557e11b16c 100755 --- a/t/t9802-git-p4-filetype.sh +++ b/t/t9802-git-p4-filetype.sh @@ -301,9 +301,18 @@ test_expect_success SYMLINKS 'empty symlink target' ' # @@ # cd "$db/depot" && - sed "/@target1/{; s/target1/@/; n; d; }" \ - empty-symlink,v >empty-symlink,v.tmp && - mv empty-symlink,v.tmp empty-symlink,v + case "$(echo empty-symlink*)" in + empty-symlink,v) + sed "/@target1/{; s/target1/@/; n; d; }" \ + empty-symlink,v >empty-symlink,v.tmp && + mv empty-symlink,v.tmp empty-symlink,v;; + empty-symlink,d) + path="empty-symlink,d/$(ls empty-symlink,d/ | tail -n1)" && + rm "$path" && + gzip </dev/null >"$path";; + *) + BUG "unhandled p4d layout";; + esac ) && ( # Make sure symlink really is empty. Asking diff --git a/t/t9825-git-p4-handle-utf16-without-bom.sh b/t/t9825-git-p4-handle-utf16-without-bom.sh index f049ff8229..8e34f72198 100755 --- a/t/t9825-git-p4-handle-utf16-without-bom.sh +++ b/t/t9825-git-p4-handle-utf16-without-bom.sh @@ -22,9 +22,21 @@ test_expect_success 'init depot with UTF-16 encoded file and artificially remove cd db && p4d -jc && # P4D automatically adds a BOM. Remove it here to make the file invalid. - sed -e "\$d" depot/file1,v >depot/file1,v.new && - mv depot/file1,v.new depot/file1,v && - printf "@$UTF16@" >>depot/file1,v && + case "$(echo depot/file1*)" in + depot/file1,v) + sed -e "\$d" depot/file1,v >depot/file1,v.new && + mv depot/file1,v.new depot/file1,v && + printf "@$UTF16@" >>depot/file1,v;; + depot/file1,d) + path="$(echo depot/file1,d/*.gz)" && + gunzip -c "$path" >"$path.unzipped" && + sed -e "\$d" "$path.unzipped" >"$path.new" && + printf "$UTF16" >>"$path.new" && + gzip -c "$path.new" >"$path" && + rm "$path.unzipped" "$path.new";; + *) + BUG "unhandled p4d layout";; + esac && p4d -jrF checkpoint.1 ) '
Some of the tests in t98xx modify the Perforce depot in ways that the tool wouldn't normally allow. This is done to test behaviour of git-p4 in certain edge cases that we have observed in the wild, but which should in theory not be possible. Naturally, modifying the depot on disk directly is quite intimate with the tool and thus prone to breakage when Perforce updates the way that data is stored. And indeed, those tests are broken nowadays with r23 of Perforce. While a file revision was previously stored as plain file "depot/file,v", it is now stored in a directory "depot/file,d" with compression. Adapt those tests to handle both old- and new-style depot layouts. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- t/t9800-git-p4-basic.sh | 13 +++++++++++-- t/t9802-git-p4-filetype.sh | 15 ++++++++++++--- t/t9825-git-p4-handle-utf16-without-bom.sh | 18 +++++++++++++++--- 3 files changed, 38 insertions(+), 8 deletions(-)