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

api fix #309

Merged
merged 5 commits into from Mar 28, 2019

Conversation

4 participants
@zekth
Copy link
Contributor

commented Mar 26, 2019

Fixing: #308

Sorry for breaking it.

{ fn, name }: TestDefinition,
exitOnFail: boolean
exitOnFail: boolean,
{ fn, name }: TestDefinition

This comment has been minimized.

Copy link
@ry

ry Mar 26, 2019

Contributor

Why didn't any tests fail before? Is there some API that's not being exercised?

cc @chiefbiiko

This comment has been minimized.

Copy link
@zekth

zekth Mar 26, 2019

Author Contributor

As it's not exported, the only way to test it would be to do an assert in runTests() for runTests(). Maybe chiefbiiko has a clue to test it?

This comment has been minimized.

Copy link
@ry

ry Mar 26, 2019

Contributor

Something isn't being exercised - otherwise #307 wouldn't have been green. We should add a test with this commit that was failing before.
Maybe @chiefbiiko can help.

This comment has been minimized.

Copy link
@chiefbiiko

chiefbiiko Mar 27, 2019

Contributor

@ry @zekth Yea, sorry, I missed adding a test for parallel execution. See patch below.

@bartlomieju

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2019

Why did CI pass in #307?

EDIT: @ry was faster...

@zekth

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2019

Just pushed a proposal for testing runTest in parallel and serial. Ensuring it runs properly and does not throw exception. Had to flush tests array in the mod to make it work.

Last improvement would be to check the Deno status code generated by Deno.exit(). Is it possible to reach it? Therefore we can check if it ouputs 0 when assert true and 1 when false.

Exposing the exit code could be a solution: https://github.com/denoland/deno/blob/master/js/os.ts#L50

It's a bit sketchy but works. As i mentionned in #307 could be refactored in a further PR. Your thoughts?

@chiefbiiko

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2019

I suggest applying below patch. It adds the missing parallel tests.

To enable running a test subset in parallel after testing all serially (in testing/main.ts) I introduced the only and skip bits to RunOptions. Couldn't use testing.setFilter to select a subset bc that filter is applied globally.

@ry @zekth @bartlomieju

diff --git a/testing/main.ts b/testing/main.ts
index 077d662..00502b2 100644
--- a/testing/main.ts
+++ b/testing/main.ts
@@ -1,3 +1,11 @@
 // Copyright 2018-2019 the Deno authors. All rights reserved. MIT license.
 import { runTests } from "./mod.ts";
-runTests();
+
+async function main() {
+  // Testing entire test suite serially
+  await runTests();
+  // Testing parallel execution on a subset that does not depend on exec order
+  await runTests({ parallel: true, only: /^testing/ });
+}
+
+main();
diff --git a/testing/mod.ts b/testing/mod.ts
index d0d755e..71f27db 100644
--- a/testing/mod.ts
+++ b/testing/mod.ts
@@ -10,7 +10,7 @@ export interface TestDefinition {
 }
 
 let filterRegExp: RegExp | null;
-const tests: TestDefinition[] = [];
+const candidates: TestDefinition[] = [];
 
 let filtered = 0;
 
@@ -35,7 +35,7 @@ export function test(t: TestDefinition | TestFunction): void {
     throw new Error("Test function may not be anonymous");
   }
   if (filter(name)) {
-    tests.push({ fn, name });
+    candidates.push({ fn, name });
   } else {
     filtered++;
   }
@@ -124,8 +124,8 @@ function previousPrinted(name: string, results: TestResults): boolean {
 async function createTestCase(
   stats: TestStats,
   results: TestResults,
-  { fn, name }: TestDefinition,
-  exitOnFail: boolean
+  exitOnFail: boolean,
+  { fn, name }: TestDefinition
 ): Promise<void> {
   const result: TestResult = results.cases.get(results.keys.get(name));
   try {
@@ -197,6 +197,8 @@ async function runTestsSerial(
 export interface RunOptions {
   parallel?: boolean;
   exitOnFail?: boolean;
+  only?: RegExp;
+  skip?: RegExp;
 }
 
 /**
@@ -205,11 +207,16 @@ export interface RunOptions {
  */
 export async function runTests({
   parallel = false,
-  exitOnFail = false
+  exitOnFail = false,
+  only = /[^\s]/,
+  skip = /^\s*$/
 }: RunOptions = {}): Promise<void> {
+  const tests: TestDefinition[] = candidates.filter(
+    ({ name }) => only.test(name) && !skip.test(name)
+  );
   const stats: TestStats = {
     measured: 0,
-    ignored: 0,
+    ignored: candidates.length - tests.length,
     filtered: filtered,
     passed: 0,
     failed: 0
import "./format_test.ts";
import "./diff_test.ts";
import "./pretty_test.ts";
import "./asserts_test.ts";
import "./bench_test.ts";

test(function testingAssertEqualActualUncoercable() {

This comment has been minimized.

Copy link
@ry

ry Mar 28, 2019

Contributor

These do need to be moved, but can you do this in a separate PR?

This comment has been minimized.

Copy link
@zekth

zekth Mar 28, 2019

Author Contributor

Yes. I'll drop this commit.

@zekth zekth force-pushed the zekth:fix_307 branch from 160973c to 14f21c7 Mar 28, 2019

zekth added some commits Mar 28, 2019

@zekth

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2019

@ry ready to merge.

@ry

ry approved these changes Mar 28, 2019

Copy link
Contributor

left a comment

LGTM

@ry ry merged commit cac060f into denoland:master Mar 28, 2019

2 checks passed

denoland.deno_std #20190328.8 succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@bartlomieju bartlomieju referenced this pull request Apr 9, 2019

Closed

Upgrade deno_std #2064

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.