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

test: Run scripted-diff in subshell #14864

Merged
merged 1 commit into from Dec 6, 2018
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -20,23 +20,23 @@ fi
RET=0
PREV_BRANCH=`git name-rev --name-only HEAD`
PREV_HEAD=`git rev-parse HEAD`
for i in `git rev-list --reverse $1`; do
if git rev-list -n 1 --pretty="%s" $i | grep -q "^scripted-diff:"; then
git checkout --quiet $i^ || exit
SCRIPT="`git rev-list --format=%b -n1 $i | sed '/^-BEGIN VERIFY SCRIPT-$/,/^-END VERIFY SCRIPT-$/{//!b};d'`"
for commit in `git rev-list --reverse $1`; do
if git rev-list -n 1 --pretty="%s" $commit | grep -q "^scripted-diff:"; then
git checkout --quiet $commit^ || exit
SCRIPT="`git rev-list --format=%b -n1 $commit | sed '/^-BEGIN VERIFY SCRIPT-$/,/^-END VERIFY SCRIPT-$/{//!b};d'`"
if test "x$SCRIPT" = "x"; then
echo "Error: missing script for: $i"
echo "Error: missing script for: $commit"
echo "Failed"
RET=1
else
echo "Running script for: $i"
echo "Running script for: $commit"
echo "$SCRIPT"
eval "$SCRIPT"
git --no-pager diff --exit-code $i && echo "OK" || (echo "Failed"; false) || RET=1
(eval "$SCRIPT")
This conversation was marked as resolved by dongcarl

This comment has been minimized.

Copy link
@practicalswift

practicalswift Dec 4, 2018

Member

Would it make sense to use (set -e; eval "$SCRIPT") || RET=1 && echo "scripted-diff execution failed."?

That would allow us to catch execution failures within the scripted-diff run. Currently such execution failure are silent (as long as the diff produces the expected result).

It probably shouldn't be done as part of this PR as that would change behaviour.

This comment has been minimized.

Copy link
@dongcarl

dongcarl Dec 4, 2018

Author Contributor

Not sure about set -e, as I've found that people are not used to its behaviour and there are legitimate uses of not failing immediately upon the failure of one command. I think that as long as the diff produces the expected result, we should be good.

What I would like to have is set -x, which doesn't change behaviour but allows us to see line by line execution fully expanded (in case it's a complex script they'd like to debug).

This comment has been minimized.

Copy link
@practicalswift

practicalswift Dec 4, 2018

Member

Yes, you have a good point. set -x would be really nice :-)

git --no-pager diff --exit-code $commit && echo "OK" || (echo "Failed"; false) || RET=1
fi
git reset --quiet --hard HEAD
else
if git rev-list "--format=%b" -n1 $i | grep -q '^-\(BEGIN\|END\)[ a-zA-Z]*-$'; then
if git rev-list "--format=%b" -n1 $commit | grep -q '^-\(BEGIN\|END\)[ a-zA-Z]*-$'; then
echo "Error: script block marker but no scripted-diff in title"
echo "Failed"
RET=1
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.