feat(unstable/test): imperative test steps API

[dsherret]

Implementation of #10771 and part of #11901.

This is missing permissions and metadata, but those can be added in a future PR (likely easier once permissions stabilize).

[caspervonb]

Nice test coverage! but this changes a lot of unnecessary things (undoing things I've more or less just refactored to be that way, like the enums) and seems overly complex.

Also #11736 has been open for a month pending consensus, looks like they're at about the same level of completeness (mine can stream output tho).

[dsherret]

but this changes a lot of unnecessary things (undoing things I've more or less just refactored to be that way, like the enums) and seems overly complex.

Could you point out the specific areas you mean in the PR by commenting on them? As for the complexity, I believe it is necessary to implement this feature based on the RFC as there are a lot of conditions that need to be enforced at runtime. I'm sure there are a few areas that could be refactored to be a little cleaner though as this is just a first pass.

Also #11736 has been open for a month pending consensus, looks like they're at about the same level of completeness (mine can stream output tho).

I did not see this PR. I'm sorry about that and feel bad... nobody told me about it. In the future though it would be helpful to open an issue for work that you're working/going to be working on to signal that you're doing that work. Then once done linking the PR to that issue. For example, I opened #11901 and assigned it to myself to signal I was going to work on this a few weeks ago... it would have been helpful to have commented on that issue with your PR to let me know that this had already been done. Though that said, I should have reached out to you about this one this week just to ensure we weren't duplicating work. Oh well... perhaps we could combine some aspects of the two PRs though.

I don't think the streaming output works properly with the approach in that PR with test steps executed in parallel. The output will trample all over itself. With this PR we could improve it to be streaming, but this could only be done reliably when a test has sanitizers and we know that it will be executing one at a time. It would then need to handle switching between the two possibilities. I think that could be a future improvement and perhaps we could use the API from your PR to help do that.

[caspervonb]

We need a different name, Tester is the name of the actual main test runner type.

[lucacasonato]

Do you have a different suggestion? TestContext?

[caspervonb]

TestContext or just Test work

[dsherret]

Renamed to TestContext.

[caspervonb]

Looking better but still, like I've been saying offline I would just go with the existing PR rather than putting in the work on making this landable.

If you're going to keep at this it would be useful to differentiate or do something different here. Maybe showcase how metadata works and how that provides us with the tight deferred test runner controlled execution can be polyfilled with that as was mentioned in #11901?

Premise put forth seems to be that we don't need tight integration with the test runner, I'd like to see that proven before allowing either steps implementations to be merged.

[dsherret]

Looking better but still, like I've been saying offline I would just go with the existing PR rather than putting in the work on making this landable.

The duplicate PR is unfortunate, but there is still some scenarios not handled by #11736 and it also has filtering which hasn't been discussed (we probably should add that in a follow up PR).

If you're going to keep at this it would be useful to differentiate or do something different here. Maybe showcase how metadata works and how that provides us with the tight deferred test runner controlled execution can be polyfilled with that as was mentioned in #11901?

Premise put forth seems to be that we don't need tight integration with the test runner, I'd like to see that proven before allowing either steps implementations to be merged.

Like you, I'm still not convinced we should go with metadata, but it would work. For now, I've moved this API to unstable.

[lucacasonato]

This is working great with my mocha polyfill:

import "https://gist.githubusercontent.com/lucacasonato/54c03bb267074aaa9b32415dbfb25522/raw/fc2cb1d656c733f76a5db6792fa3a26b9b683e33/deno_mocha.ts";

describe("group", () => {
  it("hello", () => {
    throw new Error("Hey!");
  });

  it("world", () => {
    1 + 2;
  });
});

[lucacasonato]

This looks great. The assertions to prevent footguns are all there. I'm happy with landing this in unstable.

[bartlomieju]

This is working great with my mocha polyfill:

import "https://gist.githubusercontent.com/lucacasonato/54c03bb267074aaa9b32415dbfb25522/raw/fc2cb1d656c733f76a5db6792fa3a26b9b683e33/deno_mocha.ts";

describe("group", () => {
  it("hello", () => {
    throw new Error("Hey!");
  });

  it("world", () => {
    1 + 2;
  });
});

Just FYI it seems Mocha can already work with Deno: https://github.com/denoland/deno_std/pull/1334/files#diff-fa7e75c2cff0b94ed10f5be1e8c056c56d52ca0f8fcb6e3ec3a2c2aed37291af

[lucacasonato]

Let's do it!

[bartlomieju]

LGTM

[bartlomieju]

Why is (t as any) required? I thought TS could infer that it's TestContext

[dsherret]

There's no step method in the type declarations when not in unstable.

[N8Brooks]

Is it supposed to count how many Deno.tests there are or how many t.stepss there are? Right now it seems to count how many Deno.tests there are even though there are multiple t.stepss. For example:

test day01 ...
  test part1 ...
    test example1 ... ok (8ms)
    test example2 ... ok (7ms)
    test example3 ... ok (7ms)
    test data ... ok (8ms)
  ok (40ms)
  test part2 ...
    test example1 ... ok (7ms)
    test example2 ... ok (8ms)
    test data ... ok (8ms)
  ok (31ms)
ok (79ms)

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out (105ms)

It says "1 passed" even though there are several t.stepss.

Very cool feature! This will help a lot organizing tests!

[bartlomieju]

CC @dsherret

[dsherret]

@N8Brooks technically there is only 1 test there and there are 2 steps with each their own steps underneath them. So the final count is only the tests that have passed and not how many steps there were to do that test. Perhaps #12411 would make this more clear?

[hayd]

TestContext broke tests in deno-lambda's serverless example, which was doing its own setup/teardown:

https://github.com/hayd/deno-lambda/blob/debc93d9416bc546a06e139276b57367ca45da45/example-serverless/test_util.ts#L9

Are we supposed to just stub in fn(undefined as any) ? 😳

Is there an issue/discussion for TestDefinition?

[dsherret]

@hayd you may just want to provide:

fn({
  step() {
    throw new Error("Test steps are not supported by deno-lambda at this time.");
  }
} as any)