[Deno] get code coverage working / solve rare crypto-related failures

[cinnamon-bun]

What's the problem you want solved?

deno coverage doesn't let you specify an import map, so we get this error from inside noble-ed25519:

error: Relative import path "crypto" not prefixed with / or ./ or ../
    at https://deno.land/x/[email protected]/index.ts:6:24

To reproduce this, add to the Makefile:

test-coverage:
	deno test --import-map=import_map.json --no-check=remote --config deno.json --coverage=coverage src

show-coverage:
	deno coverage coverage

Is there a solution you'd like to recommend?

Maybe we can import noble-ed25519 through skypack or esm.sh to fix this? It's also a pretty small package, we could just copy it and modify it.

[cinnamon-bun]

Importing it from https://esm.sh/@noble/[email protected] fixes some things, but not code coverage.

It replaces import "crypto" with import "https://deno.land/[email protected]/node/crypto.ts", and that means we don't need an import map anymore. You can see this by running

deno info "https://esm.sh/@noble/[email protected]"
(...)
https://esm.sh/@noble/[email protected] (113B)
├─┬ https://cdn.esm.sh/v61/@noble/[email protected]/deno/ed25519.js (10.82KB)
│ └─┬ https://deno.land/[email protected]/node/crypto.ts (5.21KB)

[cinnamon-bun]

I've committed the above change to imports. Code coverage now fails with a deno crash when it's parsing the coverage data. The tests all pass though.

> make coverage
(...)
deno coverage coverage
============================================================
Deno has panicked. This is a bug in Deno. Please report this
at https://github.com/denoland/deno/issues/new.
(...)
Platform: macos x86_64
Version: 1.17.2
Args: ["deno", "coverage", "coverage"]

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: BadJson(Error("invalid unicode code point", line: 1, column: 4469))', cli/tools/coverage.rs:202:57

[cinnamon-bun]

Also getting rare test failures where an async op wasn't finished before the test finished, somewhere in the crypto code. Maybe a result of forcing noble-ed25519 to use deno crypto instead of node crypto...

make test-coverage
(...)
StorageDriverLocalStorage + CryptoDriverNoble: storage overwriteAllDocsByAuthor
AssertionError: Test case is leaking async ops.
Before:
  - dispatched: 37
  - completed: 37
After:
  - dispatched: 64
  - completed: 63
Ops:
  op_crypto_subtle_digest:
    Before:
      - dispatched: 26
      - completed: 26
    After:
      - dispatched: 52
      - completed: 51

Make sure to await all promises returned from Deno APIs before
finishing test case.
    at assert (deno:runtime/js/06_util.js:41:13)
    at asyncOpSanitizer (deno:runtime/js/40_testing.js:122:7)
    at async resourceSanitizer (deno:runtime/js/40_testing.js:138:7)
    at async Object.exitSanitizer [as fn] (deno:runtime/js/40_testing.js:170:9)
    at async runTest (deno:runtime/js/40_testing.js:428:7)
    at async Object.runTests (deno:runtime/js/40_testing.js:541:22)

[cinnamon-bun]

noble-ed25519 is indeed calling crypto.web.subtle.digest here, and it is correctly awaiting the result:

https://github.com/paulmillr/noble-ed25519/blob/main/index.ts#L835-L839

    if (crypto.web) {
      const buffer = await crypto.web.subtle.digest('SHA-512', message.buffer);
      return new Uint8Array(buffer);
    } else if (crypto.node) {
      return Uint8Array.from(crypto.node.createHash('sha512').update(message).digest());

But! It should be using the node version of crypto, as seen by running deno info (see above). The node version in deno.land/std/node/crypto.ts is totally synchronous, there should be no chance of an async call happening...

[cinnamon-bun]

Here's the problem, noble-ed25519 is sniffing to see if it's in a browser or in Node:

https://github.com/paulmillr/noble-ed25519/blob/main/index.ts#L797-L802

// Global symbol available in browsers only. Ensure we do not depend on @types/dom
declare const self: Record<string, any> | undefined;
const crypto: { node?: any; web?: any } = {
  node: nodeCrypto,
  web: typeof self === 'object' && 'crypto' in self ? self.crypto : undefined,
};

Deno acts like a browser in this case, but we probably want it to use the node version of its code to avoid crashes.

[cinnamon-bun]

related PR: #130

[sgwilym]

I did a little investigating into this this morning.

1. I created an isolated test case where a tiny copy of crypto driver using the unmodified version of ed25519 served via deno.land was run against tests. After several attempts running this workflow on Github I could not get it to fail.
2. I created a new branch where I've undone your extraction of ed25519 into the codebase and imported the unmodified version of ed25519 served via deno.land and repeated tests on Github workflows. I cannot get them to fail after several attempts.

This makes me think the following:

• Tests began failing after importing a transformed version of ed25519 via esm.sh (in this commit) and indeed looking at this list you can see that's the case.

The good news is ed25519 works and there is no underlying async leak with Deno + linux.

---

This leaves the issue with coverage. deno coverage does not yet support import maps and that is annoying.

I have another motivation for removing the need for an import map from the project: Deno does not automatically detect import maps and we would thus be requiring all downstream users of Earthstar to have an import map for ed25519's sake. I've made an issue for that here.

I have another idea to deal with this that I'm going to try now and report back on.

[sgwilym]

Please see the above PR, which I think has unravelled this mystery.

[sgwilym]

I think this is fixed by #132. The async crypto related failures are definitely fixed, and generating coverage seems to work for me pretty consistently (the trick seems to be always running make coverage and not its subcommands independently).