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

feat(cli/js/testing): Add TestDefinition::skip #4351

Merged
merged 8 commits into from
Mar 15, 2020

Conversation

nayeemrmn
Copy link
Collaborator

@nayeemrmn nayeemrmn commented Mar 13, 2020

Closes #4098.

OUTDATED EXAMPLE: shorthands no longer support skip.
test.ts:

Deno.test({ name: "some test", fn() {
  console.log(1);
} });

Deno.test({ name: "some skipped test", skip: true, fn() {
  console.log(2);
} });

Deno.test("some skipped test (shorthand)", function abc() {
  console.log(3);
}, { skip: true });

Deno.test(function someSkippedTestShorthand2() {
  console.log(4);
}, { skip: true });

cargo run -- test test.ts:

td

cc @bartlomieju

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

@nayeemrmn looks ok, but I'm a bit hesitant to change Deno.test() signature right now

cli/js/lib.deno.ns.d.ts Show resolved Hide resolved
cli/js/testing.ts Outdated Show resolved Hide resolved
cli/js/testing.ts Show resolved Hide resolved
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM, but let's wait for @ry's opinion before landing

(It's fine to land this one before #4371)

Comment on lines +36 to +40
enum TestStatus {
Passed = "passed",
Failed = "failed",
Skipped = "skipped"
}
Copy link
Member

Choose a reason for hiding this comment

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

We should probably collapse all of little interfaces and enums for testing under a single namespace...

Copy link
Collaborator Author

@nayeemrmn nayeemrmn Mar 14, 2020

Choose a reason for hiding this comment

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

Just realised these are all implicitly public via TestReporter in RunTestsOptions so that would be necessary. The TestEvent* stuff can and should be hidden IMO with some restructuring. All that needs to be public is TestReporter, TestResult, TestStatus and TestStats each with no references to any other proprietary type.

EDIT: Actually TestEvent{Start,Result,End} are necessary to keep TestReporter simple and extensible, but not TestEvent.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How is this?

namespace tests {
  export interface Result {
    name: string;
    status: "passed" | "failed" | "skipped";
    duration?: number;
    error?: Error;
  }

  export interface Stats {
    filtered: number;
    ignored: number;
    measured: number;
    passed: number;
    failed: number;
  }

  export interface StartEvent {
    tests: Deno.TestDefinition[];
  }

  export interface ResultEvent {
    result: Result;
  }

  export interface EndEvent {
    stats: Stats;
    duration: number;
    results: Result[];
  }

  export interface Reporter {
    start(msg: StartEvent): Promise<void>;
    result(msg: ResultEvent): Promise<void>;
    end(msg: EndEvent): Promise<void>;
  }

  export class ConsoleReporter implements Reporter {
    // ...
  }
}

Copy link
Member

Choose a reason for hiding this comment

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

@nayeemrmn I already needed to split "result" into two separate messages for #4371
We should figure this out before 1.0, but it's not actionable for this PR

@bartlomieju bartlomieju requested a review from ry March 14, 2020 17:22
@bartlomieju
Copy link
Member

@nayeemrmn can you roll back changes to test_util? I think it will need some changes to work with skip from test definition and I'll incorporate them in #4371

@nayeemrmn nayeemrmn force-pushed the test-definition-skip branch from e531caa to 92a3c09 Compare March 14, 2020 18:03
@nayeemrmn nayeemrmn changed the title WIP: feat(cli/js/testing): Add TestDefinition::skip feat(cli/js/testing): Add TestDefinition::skip Mar 14, 2020
@ry
Copy link
Member

ry commented Mar 14, 2020

I'm fine with exposing "skip" but this PR conflicts with #4371. @bartlomieju - leaving this to you to figure out how to merge and in which order.

@cknight
Copy link
Contributor

cknight commented Mar 14, 2020

With realising that @nayeemrmn was already working on this, I also went to tackle adding skip to testing.ts. Oops! If it's of any use, my simplistic implementation can be found here: https://github.com/cknight/deno/compare/0df9823..skipDenoTest

I'll not take my changes any further as this PR seems to cover it.

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @nayeemrmn!

@bartlomieju bartlomieju merged commit 64a35ac into denoland:master Mar 15, 2020
@nayeemrmn nayeemrmn deleted the test-definition-skip branch March 15, 2020 09:38
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.

Add TestDefinition::skip
4 participants