Fix end-to-end test CI

[dandavison]

cc @MarcoIeni @ulwlu

This is an attempt to fix the end-to-end test CI. Currently, I believe that it is not failing even when it should fail.

[ghost]

@dandavison This is my suggestion.

Ideal

end-to-end can be executed with or without passing bin-path.

What's the problem currently

end-to-end cannot be executed without passing bin-path.

What this PR aims, and what it should be

This PR aims to fix CI to be error when it should be.

However, I think CI's alright already. It will fail when test is failed. On the other hand, bash syntax is correct for CI, It's alright that CI won't error while make end-to-end-test failing. CI passes bin-path, so it's supposed to be passed.

So, there's no need to fix ci since we fix end-to-end.

[dandavison]

However, I think CI's alright already.

I'm not sure I agree -- with the latest commit 462fc56cf4557137cfb594d81a6e6cc2e63644cc CI fails as expected:

https://github.com/dandavison/delta/runs/1494106155

but previously it was passing when it should have failed:

https://github.com/dandavison/delta/runs/1493996100

[ghost]

@dandavison

CI fails as expected

Yes, currently it fails because it should be like

# ; is required in bash not like rust. IF IT IS IN ONE LINE. It becomes one line by running with >-.
DELTA_BIN=target/x86_64-apple-darwin/release/delta; ./tests/test_raw_output_matches_git_on_full_repo_history $DELTA_BIN && ./tests/test_deprecated_options $DELTA_BIN > /dev/null

in my guess.

[ghost]

And previous one is supposed to succeed. So it's as we expected.

It passes $DELTA_BIN correctly, so assigning variable to $1 won't occur at ${1:=./target/release/delta}.

[dandavison]

; is required in bash not like rust.

@ulwlu I don't agree that ; is required -- but you made me notice an error in my code. Do you agree with this:

DELTA_BIN=foo ./do_this means:

Create a child process to run the executable "./do_this", and set the env var DELTA_BIN=foo in that child process.

Whereas DELTA_BIN=foo; ./do_this means:

Set DELTA_BIN=foo in the current shell process. Then run the executable "./do_this". But, notice that DELTA_BIN=foo will not be set in the child process.

Now, I had made a mistake! At ec18620beb1ee91773973350dbe400816821cc1d I had this:

DELTA_BIN=foo ./do_this && ./do_that. But that means:

Create child process 1 running executable "./do_this" with the env var set in that child process. Then, if that succeeds create child process 2 running executable "./do_that" but without setting the env var.

So that wasn't what we wanted. I've switched it to use make now as I think we can rely on a suitable make being present in MacOS and Linux build envs, and this way we can add to the make target without having to duplicate that in the CI config.

[ghost]

@dandavison I thought ./{script} is same as source ./{script}, so they should be in same process? If we do like bash -c './{script}', then it will create another process.

I've switched it to use make now as I think we can rely on a suitable make being present in MacOS and Linux build envs, and this way we can add to the make target without having to duplicate that in the CI config.

Yes! I see it works fine.

BTW,

　I don't agree that ; is required

This actually is, below is the example.

An article about similar topic is here https://unix.stackexchange.com/questions/56444/how-do-i-set-an-environment-variable-on-the-command-line-and-have-it-appear-in-c.

[dandavison]

This actually is, below is the example.

Oh I see! Yes you're right, it was being passed as an argument. I forgot about that, and was thinking about the default assignment construct in the shell script.

[dandavison]

I thought ./{script} is same as source ./{script}, so they should be in same process?

No, that's not true. ./script runs an executable named "script" as a child process. The "executable" might happen to be a shell script with a shebang line (#!/bin/bash or #!/some/interpreter) but that doesn't change things -- "some/interpreter" is started as a new child process and it is given the contents of the script.

~ cat print-pid
#!/bin/bash
echo $$
~ source print-pid 
5292
~ source print-pid
5292
~ ./print-pid
60932
~ ./print-pid
60954
~ bash ./print-pid 
60992
~ source print-pid
5292

[ghost]

~ cat print-pid #!/bin/bash echo $$ ~ source print-pid 5292 ~ source print-pid 5292 ~ ./print-pid 60932 ~ ./print-pid 60954 ~ bash ./print-pid 60992 ~ source print-pid 5292

@dandavison Oh sorry, you are correct. I made mistake for the dot below.

# These are same.
source script.sh
. script.sh

# This creates new shell
./script.sh

[dandavison]

Oh right, sorry, you already knew what I wrote :)

Yeah, incidentally I switched to zsh recently and I don't think the . script syntax exists; just source.

[ghost]

you already knew what I wrote :)

I completely forgot and learn again now thank to you.

With the fix, I can start moving on https://github.com/dandavison/delta/issues/405 again. Thank you very much!

[dandavison]

I haven't quite got a correct version of this. The point I've got to is that I don't understand this error output. When this happens, the integration tests don't really run, but they also don't fail.

shell: /bin/bash -e {0}

For example https://github.com/dandavison/delta/runs/1494765037

[dandavison]

OK, the current commit is the closest I've got to the behaviour we want with the deliberate error that's in this branch. It's need one more tweak though as I don't think it's setting the env var correctly in both processes.

[marcoieni]

Sorry I haven't read all the discussion in detail. Are you trying to setup an environment variable in a GitHub action step?

In this case that's how you do it:

https://github.com/dandavison/delta/blob/0ddd0648190f2d0497a7f3a9e89b26d04186feaf/.github/workflows/cd.yml#L53-L57

as you can see here we set a bunch of environment variables and they are visible from the script!

[dandavison]

Sorry I haven't read all the discussion in detail. Are you trying to setup an environment variable in a GitHub action step?

Haha don't worry -- this PR is completely incomprehensible :)

The problem that it's addressing is that the end-to-end tests are passing in Github Actions CI even when there is an error in them. The error is fixed in master (ed39e4c8137e9d760eee49ac346603354902eeb4), but in this branch I'm trying to change things so the end-to-end tests will fail in Github Actions CI if an error is introduced again.

[dandavison]

I'm wondering whether the problem (shell: /bin/bash -e {0}) is that && is not being parsed correctly (it's in shell code in YAML). But we need to ensure that the test fails if either of those shell scripts exit with an error status. As far as I know, this will succeed if the first shell script fails and the second one succeeds:

        run: |
          DELTA_BIN=target/${{ matrix.target }}/release/delta
          ./tests/test_raw_output_matches_git_on_full_repo_history $DELTA_BIN
          ./tests/test_deprecated_options $DELTA_BIN > /dev/null

[marcoieni]

If we want to avoid using && we could try with set -e

https://docs.github.com/en/free-pro-team@latest/actions/reference/workflow-syntax-for-github-actions#using-a-specific-shell

It should be enough to specify bash as shell, because that option is implied

[dandavison]

Thanks @MarcoIeni. So looks like bash is the default on non-Windows builds, and by default they use set -e semantics, so what you had in the CI should be perfectly correct then. It's not currently failing when the scripts error but I'll get there eventually :)