| Message ID | 084822126301c0e723155fd76942f2e259b77158.1689960606.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | commit-graph: test cleanup and modernization | expand |
On Fri, Jul 21, 2023 at 1:31 PM Taylor Blau <me@ttaylorr.com> wrote: > The `graph_read_expect()` function is used to ensure that the output of > the "read-graph" test helper matches certain parameters (e.g., how many > commits are in the graph, which chunks were written, etc.). > > It expects the Git repository being tested to be at the current working > directory. However, a handful of t5318 tests use different repositories > stored in sub-directories. To work around this, several tests in t5318 > change into the relevant repository outside of a sub-shell, altering the > context for the rest of the suite. > > Prepare to remove these globally-scoped directory changes by teaching > `graph_read_expect()` to take an optional "-C dir" to specify where the > repository containing the commit-graph being tested is. > > Signed-off-by: Taylor Blau <me@ttaylorr.com> > --- > diff --git a/t/lib-commit-graph.sh b/t/lib-commit-graph.sh > @@ -32,6 +32,13 @@ graph_git_behavior() { > graph_read_expect() { > + DIR="." > + if test "$1" = -C > + then > + shift > + DIR="$1" > + shift > + fi > @@ -47,12 +54,15 @@ graph_read_expect() { > - cat >expect <<- EOF > + cat >$DIR/expect <<- EOF It may not matter for any of the current callers, but we'd normally want to quote the expansion of $DIR. Also, as I recall, some versions of bash complain if the target of '>' is not quoted. So: cat >"$DIR/expect" <<-EOF > header: 43475048 1 $(test_oid oid_version) $NUM_CHUNKS 0 > num_commits: $1 > chunks: oid_fanout oid_lookup commit_metadata$OPTIONAL > options:$OPTIONS > EOF > - test-tool read-graph >output && > - test_cmp expect output > + ( > + cd "$DIR" && > + test-tool read-graph >output && > + test_cmp expect output > + ) > }
On Fri, Jul 21, 2023 at 01:41:06PM -0400, Eric Sunshine wrote: > > diff --git a/t/lib-commit-graph.sh b/t/lib-commit-graph.sh > > @@ -32,6 +32,13 @@ graph_git_behavior() { > > graph_read_expect() { > > + DIR="." > > + if test "$1" = -C > > + then > > + shift > > + DIR="$1" > > + shift > > + fi > > @@ -47,12 +54,15 @@ graph_read_expect() { > > - cat >expect <<- EOF > > + cat >$DIR/expect <<- EOF > > It may not matter for any of the current callers, but we'd normally > want to quote the expansion of $DIR. Also, as I recall, some versions > of bash complain if the target of '>' is not quoted. So: > > cat >"$DIR/expect" <<-EOF Hmm. I'm certainly happy to make this change, but there are many other spots within our tests that would need similar updates. Looking through the output of: $ git grep -E '>\$[[:alnum:]_]+/.*' -- t/**/*.sh I see 25 such instances (including this one) that would need similar treatment. Thanks, Taylor
Taylor Blau <me@ttaylorr.com> writes: > On Fri, Jul 21, 2023 at 01:41:06PM -0400, Eric Sunshine wrote: >> > diff --git a/t/lib-commit-graph.sh b/t/lib-commit-graph.sh >> > @@ -32,6 +32,13 @@ graph_git_behavior() { >> > graph_read_expect() { >> > + DIR="." >> > + if test "$1" = -C >> > + then >> > + shift >> > + DIR="$1" >> > + shift >> > + fi >> > @@ -47,12 +54,15 @@ graph_read_expect() { >> > - cat >expect <<- EOF >> > + cat >$DIR/expect <<- EOF >> >> It may not matter for any of the current callers, but we'd normally >> want to quote the expansion of $DIR. Also, as I recall, some versions >> of bash complain if the target of '>' is not quoted. So: >> >> cat >"$DIR/expect" <<-EOF Correct. Documentation/CodingGuidelines spells out this shell redirection rule and it applies not just to tests but our scripted porcelains (if any remains, that is). > Hmm. I'm certainly happy to make this change, but there are many other > spots within our tests that would need similar updates. Looking through > the output of: > > $ git grep -E '>\$[[:alnum:]_]+/.*' -- t/**/*.sh > > I see 25 such instances (including this one) that would need similar > treatment. Let's not make things worse. Thanks.
diff --git a/t/lib-commit-graph.sh b/t/lib-commit-graph.sh index 5d79e1a4e96..c50553df0ed 100755 --- a/t/lib-commit-graph.sh +++ b/t/lib-commit-graph.sh @@ -32,6 +32,13 @@ graph_git_behavior() { graph_read_expect() { OPTIONAL="" NUM_CHUNKS=3 + DIR="." + if test "$1" = -C + then + shift + DIR="$1" + shift + fi if test -n "$2" then OPTIONAL=" $2" @@ -47,12 +54,15 @@ graph_read_expect() { then OPTIONS=" read_generation_data" fi - cat >expect <<- EOF + cat >$DIR/expect <<- EOF header: 43475048 1 $(test_oid oid_version) $NUM_CHUNKS 0 num_commits: $1 chunks: oid_fanout oid_lookup commit_metadata$OPTIONAL options:$OPTIONS EOF - test-tool read-graph >output && - test_cmp expect output + ( + cd "$DIR" && + test-tool read-graph >output && + test_cmp expect output + ) }
The `graph_read_expect()` function is used to ensure that the output of the "read-graph" test helper matches certain parameters (e.g., how many commits are in the graph, which chunks were written, etc.). It expects the Git repository being tested to be at the current working directory. However, a handful of t5318 tests use different repositories stored in sub-directories. To work around this, several tests in t5318 change into the relevant repository outside of a sub-shell, altering the context for the rest of the suite. Prepare to remove these globally-scoped directory changes by teaching `graph_read_expect()` to take an optional "-C dir" to specify where the repository containing the commit-graph being tested is. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- t/lib-commit-graph.sh | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)