Skip to content

Complete StarlingMonkey SDK Tests #807

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 6 commits into from
Jun 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 79 additions & 3 deletions .github/workflows/starlingmonkey.yml
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,88 @@ jobs:
rustup toolchain install 1.77.1
rustup target add wasm32-wasi --toolchain 1.77.1
- name: Build
run: npm run build:starlingmonkey:debug
run: npm run build:starlingmonkey
- uses: actions/upload-artifact@v3
with:
name: starling-debug
name: starling-release
path: starling.wasm

ensure_cargo_installs:
name: Ensure that all required "cargo install" commands are run, or we have a cache hit
strategy:
matrix:
include:
- crate: viceroy
version: 0.9.4 # Note: workflow-level env vars can't be used in matrix definitions
options: ""
- crate: wasm-tools
version: 1.0.28 # Note: workflow-level env vars can't be used in matrix definitions
options: ""
runs-on: ubuntu-latest
steps:
- name: Cache ${{ matrix.crate }} ${{ matrix.version }}
id: cache-crate
uses: actions/cache@v3
with:
path: "/home/runner/.cargo/bin/${{ matrix.crate }}"
key: crate-cache-${{ matrix.crate }}-${{ matrix.version }}
- name: Install ${{ matrix.crate }} ${{ matrix.version }}
run: cargo install ${{ matrix.crate }} ${{ matrix.options }} --version ${{ matrix.version }}
if: steps.cache-crate.outputs.cache-hit != 'true'

run_wpt:
if: github.ref != 'refs/heads/main' && false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This adds the WPT test suite to CI, but disabled for now as there are still 100 tests failing. Will post fixes in a follow-up.

name: Run Web Platform Tests
needs: [build, ensure_cargo_installs]
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
with:
submodules: true
- uses: actions/setup-node@v3
with:
node-version: 20
cache: 'yarn'

- name: Download Engine
uses: actions/download-artifact@v3
with:
name: starling-release

- name: Restore Viceroy from cache
uses: actions/cache@v3
with:
path: "/home/runner/.cargo/bin/viceroy"
key: crate-cache-viceroy-${{ env.viceroy_version }}

- name: Restore wasm-tools from cache
uses: actions/cache@v3
id: wasm-tools
with:
path: "/home/runner/.cargo/bin/wasm-tools"
key: crate-cache-wasm-tools-${{ env.wasm-tools_version }}

- name: "Check wasm-tools has been restored"
if: steps.wasm-tools.outputs.cache-hit != 'true'
run: |
echo "wasm-tools was not restored from the cache"
echo "bailing out from the build early"
exit 1

- run: yarn install --frozen-lockfile

- name: Build WPT runtime
run: tests/wpt-harness/build-wpt-runtime.sh --starlingmonkey

- name: Prepare WPT hosts
run: |
cd tests/wpt-harness/wpt
./wpt make-hosts-file | sudo tee -a /etc/hosts

- name: Run tests
timeout-minutes: 20
run: node ./tests/wpt-harness/run-wpt.mjs --starlingmonkey -vv

sdktest:
if: github.ref != 'refs/heads/main'
runs-on: ubuntu-latest
Expand Down Expand Up @@ -76,7 +152,7 @@ jobs:
- name: Download Engine
uses: actions/download-artifact@v3
with:
name: starling-debug
name: starling-release
- run: yarn install --frozen-lockfile

- name: Yarn install
Expand Down

This file was deleted.

35 changes: 24 additions & 11 deletions integration-tests/js-compute/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ let args = argv.slice(2);

const local = args.includes('--local');
const starlingmonkey = args.includes('--starlingmonkey');
const filter = args.filter(arg => !arg.startsWith('--'));

async function $(...args) {
return await retry(10, () => zx(...args))
Expand Down Expand Up @@ -111,11 +112,6 @@ core.endGroup()

let { default: tests } = await import(join(fixturePath, 'tests.json'), { with: { type: 'json' } });

if (starlingmonkey) {
const { default: testsSkipStarlingMonkey } = await import(join(fixturePath, 'tests-skip-starlingmonkey.json'), { with: { type: 'json' } });
tests = Object.fromEntries(Object.entries(tests).filter(([key]) => !testsSkipStarlingMonkey.includes(key)));
}

core.startGroup('Running tests')
function chunks(arr, size) {
const output = [];
Expand All @@ -128,6 +124,14 @@ function chunks(arr, size) {
let results = [];
for (const chunk of chunks(Object.entries(tests), 100)) {
results.push(...await Promise.allSettled(chunk.map(async ([title, test]) => {
// basic test filtering
if (!title.includes(filter)) {
return {
title,
test,
skipped: true
};
}
if (local) {
if (test.environments.includes("viceroy")) {
let path = test.downstream_request.pathname;
Expand All @@ -142,6 +146,7 @@ for (const chunk of chunks(Object.entries(tests), 100)) {
await compareDownstreamResponse(test.downstream_response, response);
return {
title,
test,
skipped: false
};
} catch (error) {
Expand All @@ -150,6 +155,7 @@ for (const chunk of chunks(Object.entries(tests), 100)) {
} else {
return {
title,
test,
skipped: true
}
}
Expand All @@ -168,6 +174,7 @@ for (const chunk of chunks(Object.entries(tests), 100)) {
await compareDownstreamResponse(test.downstream_response, response);
return {
title,
test,
skipped: false
};
} catch (error) {
Expand All @@ -177,6 +184,7 @@ for (const chunk of chunks(Object.entries(tests), 100)) {
} else {
return {
title,
test,
skipped: true
}
}
Expand All @@ -191,6 +199,7 @@ let passed = 0;
const failed = [];
const green = '\u001b[32m';
const red = '\u001b[31m';
const reset = '\u001b[0m';
const white = '\u001b[39m';
const info = '\u2139';
const tick = '\u2714';
Expand All @@ -199,16 +208,20 @@ for (const result of results) {
if (result.status === 'fulfilled') {
passed += 1;
if (result.value.skipped) {
if (local) {
console.log(white, info, `Skipped as test marked to only run on Fastly Compute: ${result.value.title}`, white);
if (!result.value.title.includes(filter)) {
console.log(white, info, `Skipped by test filter: ${result.value.title}`, reset);
} else if (local && !result.value.test.environments.includes("viceroy")) {
console.log(white, info, `Skipped as test marked to only run on Fastly Compute: ${result.value.title}`, reset);
} else if (!local && !result.value.test.environments.includes("compute")) {
console.log(white, info, `Skipped as test marked to only run on local server: ${result.value.title}`, reset);
} else {
console.log(white, info, `Skipped as test marked to only run on local server: ${result.value.title}`, white);
console.log(white, info, `Skipped due to no environments set: ${result.value.title}`, reset);
}
} else {
console.log(green, tick, result.value.title, white);
console.log(green, tick, result.value.title, reset);
}
} else {
console.log(red, cross, result.reason, white);
console.log(red, cross, result.reason, reset);
failed.push(result.reason)
}
}
Expand All @@ -219,7 +232,7 @@ if (failed.length) {
core.startGroup('Failed tests')

for (const result of failed) {
console.log(red, cross, result, white);
console.log(red, cross, result, reset);
}

core.endGroup()
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
"scripts": {
"test": "npm run test:types && npm run test:cli",
"test:cli": "brittle --bail integration-tests/cli/**.test.js",
"test:integration": "node ./integration-tests/js-compute/test.js",
"test:types": "tsd",
"build": "make -j8 -C runtime/js-compute-runtime && cp runtime/js-compute-runtime/*.wasm .",
"build:debug": "DEBUG=true make -j8 -C runtime/js-compute-runtime && cp runtime/js-compute-runtime/*.wasm .",
Expand Down
2 changes: 1 addition & 1 deletion runtime/StarlingMonkey
1 change: 0 additions & 1 deletion runtime/fastly/builtins/cache-core.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -554,7 +554,6 @@ bool CacheEntry::body(JSContext *cx, unsigned argc, JS::Value *vp) {
// options parameter is optional
// options is meant to be an object with an optional `start` and `end` fields, both which can be
// Numbers.
ENGINE->dump_value(options_val, stdout);
if (!options_val.isUndefined()) {
if (!options_val.isObject()) {
JS_ReportErrorASCII(cx, "options argument must be an object");
Expand Down
4 changes: 1 addition & 3 deletions runtime/fastly/builtins/fetch/request-response.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -228,9 +228,7 @@ bool RequestOrResponse::process_pending_request(JSContext *cx, FastlyHandle hand
}

bool RequestOrResponse::is_instance(JSObject *obj) {
return Request::is_instance(obj) || Response::is_instance(obj) ||
SimpleCacheEntry::is_instance(obj) || KVStoreEntry::is_instance(obj) ||
CacheEntry::is_instance(obj);
return Request::is_instance(obj) || Response::is_instance(obj) || KVStoreEntry::is_instance(obj);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why remove SimpleCacheEntry and CacheEntry?

Copy link
Contributor Author

@guybedford guybedford Jun 20, 2024

Choose a reason for hiding this comment

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

Right now, the js-compute-runtime build does not test its own debug build for the SDK tests, and I originally added these to the StarlingMonkey port fix assertions that were failing for the debug build, but it turns out it changes streaming behaviour which was the root cause of the remaining test. Thus switching from the debug build to the release build and reverting these changes was the required fix.

We should follow-up with a separate PR to reenable debug builds for the SDK tests, where we should then fix the assertions both for the existing runtime and the StarlingMonkey port.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok. Thanks!

}

uint32_t RequestOrResponse::handle(JSObject *obj) {
Expand Down
5 changes: 5 additions & 0 deletions runtime/fastly/handler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,11 @@ void handle_incoming(host_api::Request req) {
if (JS_IsExceptionPending(ENGINE->cx())) {
ENGINE->dump_pending_exception("evaluating code");
} else if (!success) {
if (ENGINE->has_pending_async_tasks()) {
fprintf(stderr, "Event loop terminated with async tasks pending. "
"Use FetchEvent#waitUntil to extend the service's lifetime "
"if needed.\n");
}
abort();
}

Expand Down
1 change: 0 additions & 1 deletion runtime/js-compute-runtime/builtins/cache-core.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -548,7 +548,6 @@ bool CacheEntry::body(JSContext *cx, unsigned argc, JS::Value *vp) {
// options parameter is optional
// options is meant to be an object with an optional `start` and `end` fields, both which can be
// Numbers.
dump_value(cx, options_val, stdout);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems like a log that was included previously as an oversight.

if (!options_val.isUndefined()) {
if (!options_val.isObject()) {
JS_ReportErrorASCII(cx, "options argument must be an object");
Expand Down
2 changes: 1 addition & 1 deletion tests/wpt-harness/build-wpt-runtime.sh
100644 → 100755
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,4 @@ inputs=(
)

cat "${inputs[@]}" > "${script_dir}/wpt-test-runner.js"
node "${script_dir}/../../js-compute-runtime-cli.js" "${script_dir}/wpt-test-runner.js" wpt-runtime.wasm
node "${script_dir}/../../js-compute-runtime-cli.js" "${script_dir}/wpt-test-runner.js" "$@" wpt-runtime.wasm
Loading