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

Add workerd test for running unit tests under workerd #341

Merged
merged 11 commits into from
Feb 17, 2023

Conversation

kentonv
Copy link
Member

@kentonv kentonv commented Feb 2, 2023

Tests are written by defining an entrypoint with a method test(), similar to fetch():

export let mathTest = {
  async test(controller, env, ctx) {
    if (1 + 1 != 2) {
      throw new Error("math is broken");
    }
  }
}

Next, define a normal workerd configuration with this file as a service.

Then, do:

workerd test conf.capnp

All exported test() handlers will run. The test passes if none of them throw an exception.

Check out the sample included in this PR for a bit more detail.

Additionally, this PR adds a wd_test Bazel rule and even uses it to bring in one of our (previously-internal) API tests (blob-test).

TODO (in future PRs):

  • The test log output is serviceable but very ugly at present.
  • It's annoying for each test to be composed of two files, a capnp file and a js file. Can we either:
    • Auto-generate the capnp file? But what if special bindings are needed?
    • Create a new syntax that allows both to be specified in one file? Perhaps we should extend the capnp language to support a convenient form of large embedded text blobs? (The backtick-prefixed form is still a bit awkward for embedding whole files.)
  • There is no support for tests in service workers syntax. Does this matter? Probably not.
  • We need a convenient way to mock out fetch(). Should this be built-in, or a library? Probably a library, where you configure the global fetch() to loop back to a separate test entrypoint that implements the mock?
  • How exactly should running tests under the inspector work? Do we need to pause and wait for the inspector to connect, or something?

build/wd_test.bzl Outdated Show resolved Hide resolved
build/wd_test.bzl Outdated Show resolved Hide resolved
src/workerd/api/blob-test.js Outdated Show resolved Hide resolved

const unitTests :Workerd.Config = (
services = [
( name = "blob-test",
Copy link
Collaborator

Choose a reason for hiding this comment

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

do you think we can generate this file in most cases? Would be great to create just 1 file for test rather than two.

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought a lot about this. Many tests will need to customize the configuration, e.g. to test bindings, set up actors, or customize compat flags. I don't think we can avoid having some sort of way to specify configuration.

But I agree it would be nice to have the code and config in a single file. This is one of my future TODO points in the PR description.

One idea I've had is to introduce a new block-quote syntax in Cap'n Proto that is convenient for quoting large blocks of text, so that you could the JavaScript conveniently in the capnp file, like:

using Workerd = import "/workerd/workerd.capnp";

const unitTests :Workerd.Config = (
  services = [
    ( name = "blob-test",
      worker = (
        modules = [
          (name = "worker", esModule = .code)
        ],
        compatibilityDate = "2023-01-15",
      )
    ),
  ],
);

const code :Text = """
// JavaScript code here!
export default {
  async test(ctrl, env, ctx) {
    // ...
  }
}
"""

However, this might have some challenges when it comes to things like syntax highlighting?

Thoughts?

Copy link
Collaborator

@dom96 dom96 Feb 7, 2023

Choose a reason for hiding this comment

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

You could solve the syntax highlighting challenges by allowing something like this in capnp:

const code :Text = js"""
// JavaScript code here!
export default {
  async test(ctrl, env, ctx) {
    // ...
  }
}
"""

i.e. lang prefixes for string literals.

Though I personally don't think having two separate files is a big deal.

// Implements glob filters. Copied from kj/test.{h,c++}, modified only to avoid copying the
// pattern in the constructor.
//
// TODO(cleanup): Should this be a public API in KJ?
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not simply use <regex>?

Copy link
Member Author

Choose a reason for hiding this comment

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

Globs are much easier to read and write than regexes, and I don't think this use case really needs the power of regexes.

@kentonv kentonv force-pushed the kenton/test-entrypoint branch 3 times, most recently from 80aa2a2 to 379abeb Compare February 7, 2023 20:28
@kentonv
Copy link
Member Author

kentonv commented Feb 7, 2023

Ugh why does GitHub collapse force-pushes so stupidly?

Here's the changes I made since everyone reviewed:

https://github.com/cloudflare/workerd/compare/075ad9be198e638a10aa24a41ba1b0dc5599dddb..80aa2a2446679504b8d123e58a25b9c5545fc1d1

I also rebased on main.

@jasnell
Copy link
Member

jasnell commented Feb 8, 2023

Still LGTM

@kentonv
Copy link
Member Author

kentonv commented Feb 8, 2023

(rebased on main)

This method is overriding a virtual method from a privately-inherited interface (`WorkerInterface`), hence it should be private.
This checks if the service can handle a particular event type. This will be used to check for the presence of test handlers.
The handler is expected to throw an exception on failure.

Example:

```
export default {
  async test(controller, env, ctx) {
    if (1 + 1 != 2) {
      throw new Error("math is broken");
    }
  }
}
```
We'll want to reuse `startServices()` when running tests, but we don't want to listen on sockets in that case.
blob-test.js is copied directly from our internal test suite, but adapted to use the new `test()` entrypoint.

TODO: It's annoying that we need two files for a test. Perhaps the `.capnp` format could be extended with yet another way of writing strings that lets the JavaScript code be embedded in the capnp file in a way that is convenient to read and edit.
Otherwise, when using the Ekam build system, Ekam will automatically try to compile the `.capnp` files using the capnp code generator. This doesn't work for two reasons:

1. Normally `.capnp` files require a file ID. `workerd` configs are unusual in not requiring one.
2. The import path `/workerd/workerd.capnp` doesn't actually exist -- the real path in the code is `/workerd/server/workerd.capnp`.
This is needed to make ASAN builds work. Our internal CI runs all tests under ASAN automatically, including these new wd_test rules.
@kentonv kentonv merged commit 35f6e03 into main Feb 17, 2023
@kentonv kentonv deleted the kenton/test-entrypoint branch February 17, 2023 15:46
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.

5 participants