Skip to content

Allow SAFE_RUN to correctly run and check results #6128

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

Merged
merged 1 commit into from
Jun 25, 2019
Merged

Allow SAFE_RUN to correctly run and check results #6128

merged 1 commit into from
Jun 25, 2019

Conversation

koalaman
Copy link
Contributor

Hi, I came across this shell function in test/native-tests/test_native.sh:

SAFE_RUN() {
    local SF_RETURN_VALUE=$($1 2>&1)

    if [[ $? != 0 ]]; then
        >&2 echo $SF_RETURN_VALUE
        exit 1
    fi
}

invoked as e.g.

SAFE_RUN `cd $TEST_PATH; ${CH_DIR} Platform.js > Makefile`

There are multiple issues that prevent it from living up to its name:

  1. The code to run is executed by the backticks before the function ever starts:
$ test_run() { true; }
$ test_run `ls /doesnotexist`
ls: cannot access '/doesnotexist': No such file or directory
  1. If the invocation is changed to pass the command directly, the invocation fails for metacharacters like ; and >. Here they are both treated as literal text:
$ test_run() { local var=$($1 >&2); echo "$var"; }
$ test_run "echo Hello; ls > filelist"
Hello; ls > filelist
  1. If passed a valid invocation, local masks all exit codes and returns success, even when the command fails:
$ test_run() { local foo=$($1); echo  "$1 returned $?"; }
$ test_run true
true returned 0
$ test_run false
false returned 0

The suggested commits fixes all these issues.

I was unable to verify the fix locally as the tests appeared to hang when run overnight with and without these fixes. I'm not quite sure how they're used, but there is some possibility that this commit uncovers failures that were previously hidden.

@LouisLaf LouisLaf requested a review from atulkatti May 28, 2019 19:47
Copy link
Contributor

@atulkatti atulkatti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@koalaman Changes look good to me. This script is used to run tests in our cross platform test runs. I will start the process to rebase and validate this again. If everything goes as expected the change will get pushed to GitHub through our internal tooling. Thanks for working on this. Much appreciated.

@atulkatti atulkatti self-assigned this Jun 24, 2019
@atulkatti
Copy link
Contributor

@koalaman Can you please rebase and push your branch again?

This shell function had several issues that prevented it from living up
to its name:

  1. It used `$1` to run commands, which fails to handle e.g. `;`
  1. It assigned using `local`, which would mask any errors
  3. It was invoked with command expansions, so anything it was supposed
     to run was already finished *before* it ran.
@koalaman
Copy link
Contributor Author

@atulkatti Done

@chakrabot chakrabot merged commit c2fc081 into chakra-core:master Jun 25, 2019
chakrabot pushed a commit that referenced this pull request Jun 25, 2019
…ults

Merge pull request #6128 from koalaman:master

Hi, I came across [this shell function](https://github.com/microsoft/ChakraCore/blob/68d19b73e4c796540d2d44cc17ac2320c7576cd0/test/native-tests/test_native.sh#L30) in `test/native-tests/test_native.sh`:

```
SAFE_RUN() {
    local SF_RETURN_VALUE=$($1 2>&1)

    if [[ $? != 0 ]]; then
        >&2 echo $SF_RETURN_VALUE
        exit 1
    fi
}
```

invoked as e.g.

```
SAFE_RUN `cd $TEST_PATH; ${CH_DIR} Platform.js > Makefile`
```

There are multiple issues that prevent it from living up to its name:

1.  The code to run is executed by the backticks before the function ever starts:
```
$ test_run() { true; }
$ test_run `ls /doesnotexist`
ls: cannot access '/doesnotexist': No such file or directory
```

2. If the invocation is changed to pass the command directly, the invocation fails for metacharacters like `;` and `>`. Here they are both treated as literal text:
```
$ test_run() { local var=$($1 >&2); echo "$var"; }
$ test_run "echo Hello; ls > filelist"
Hello; ls > filelist
```

3. If passed a valid invocation, `local` [masks all exit codes](https://github.com/koalaman/shellcheck/wiki/SC2155) and returns success, even when the command fails:

```
$ test_run() { local foo=$($1); echo  "$1 returned $?"; }
$ test_run true
true returned 0
$ test_run false
false returned 0
```

The suggested commits fixes all these issues.

I was unable to verify the fix locally as the tests appeared to hang when run overnight with and without these fixes. I'm not quite sure how they're used, but there is some possibility that this commit uncovers failures that were previously hidden.
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.

3 participants