Bug 276081 – run-webkit-tests failing doesn't fail EWS

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Sam Sneddon [:gsnedders]]

c.f. https://github.com/WebKit/WebKit/pull/30272#issuecomment-2198130465

https://ews-build.webkit.org/#/builders/42/builds/30643 shows layout-tests passing, despite the logs containing:

webkitpy.layout_tests.servers.http_server_base.ServerError: WPT Server process exited prematurely with status code 1

We should instead be failing the EWS test job as run-webkit-tests should be exiting with non-zero.

[Radar WebKit Bug Importer]

<rdar://problem/130908663>

[Sam Sneddon [:gsnedders]]

Looking at the code run-webkit-tests should definitely be exiting with non-zero.

However, the problem here might be the fact that we have logged:

/bin/sh -c 'python3 Tools/Scripts/run-webkit-tests --no-build --no-show-results --no-new-test-results --clobber-old-results --release --ios-simulator --results-directory layout-test-results --debug-rwt-logging --exit-after-n-failures 60 --skip-failing-tests --child-processes=5 imported/w3c/web-platform-tests 2>&1 | Tools/Scripts/filter-test-logs layout'
 in dir /Volumes/Data/worker/iOS-17-Simulator-WPT-WK2-Tests-EWS/build (timeout 19800.0 secs)
 watching logfiles {'json': 'layout-test-results/full_results.json'}
 argv: [b'/bin/sh', b'-c', b'python3 Tools/Scripts/run-webkit-tests --no-build --no-show-results --no-new-test-results --clobber-old-results --release --ios-simulator --results-directory layout-test-results --debug-rwt-logging --exit-after-n-failures 60 --skip-failing-tests --child-processes=5 imported/w3c/web-platform-tests 2>&1 | Tools/Scripts/filter-test-logs layout']
 using PTY: False
program finished with exit code 0
elapsedTime=25.727919

Because we're running a sub-shell, we're actually just getting the exit code of `sh`, and running without `-e`.

This makes this seem like potentially a regression from 279663@main, bug 270651, "Upload layout-test logs to S3".

[Sam Sneddon [:gsnedders]]

Pull request: https://github.com/WebKit/WebKit/pull/30424

[Sam Sneddon [:gsnedders]]

I'd held off from landing this because I was a bit concerned that Bug 277654 would mean this wouldn't have actually fixed the original bug, but it would, so I guess we land that and treat that as a separate issue.

[EWS]

Committed 281869@main (44f912123399): <https://commits.webkit.org/281869@main>

Reviewed commits have been landed. Closing PR #30424 and removing active labels.

[Elliott Williams]

*** Bug 277654 has been marked as a duplicate of this bug. ***

[Ryan Haddad]

This change was reverted, reopening to reflect that.

[Ryan Haddad]

*** Bug 284659 has been marked as a duplicate of this bug. ***

[Sam Sneddon [:gsnedders]]

(In reply to Angelos Oikonomopoulos from comment #4 on bug 277749)
> We got bitten by this again, with `run-jsc-stress-tests` throwing an
> exception and `filter-test-logs` accidentally eating it up, presumably
> because it relied on `-o pipefail`. I've asked internally at Igalia and all
> our buildbots should have `bash` in the PATH. For the post-commit bots could
> we change `remote_command` in `build-webkit-org/steps.py` to use `bash`,
> `-c` instead of `/bin/sh`, `-c`? Would that work on other platforms?

This is probably an acceptable option for now; we probably should longer term try to move back to `sh`, given shells are slowly adding it after its addition in POSIX 2024.

[Angelos Oikonomopoulos]

Yah, `-o pipefail` is going to be in the version of dash included in the next debian release (to be released in a few months), but I'm not sure when we'll be able to move to it. I don't think we want to wait that long to introduce `-o pipefail` anyway, so perhaps we can re-land the above commit with the command set to `bash`?

[Sam Sneddon [:gsnedders]]

Pull request: https://github.com/WebKit/WebKit/pull/44077

[EWS]

Committed 294384@main (afddd6334cd1): <https://commits.webkit.org/294384@main>

Reviewed commits have been landed. Closing PR #44077 and removing active labels.