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

[Testing] Pretty output + Silent mode #314

Merged
merged 12 commits into from Jun 19, 2019

Conversation

5 participants
@zekth
Copy link
Contributor

commented Mar 29, 2019

Following : #306

Here is a proposal of pretty output for tests (for more readability):
2019-03-29 14_14_59-MINGW64__c_projects_deno_std

Also added the option for disableLogs in the test. It is usefull for CI when there is still debug in the tests, then you directly have information of which tests is buggy and you're not flooded with all the debug logs. Currently it is set to false and only work for serialTesting. Do you think this is usefull and should i expand it to parallel? (needs a bit more refactor)

EDIT

I've added the timers on each tests and on the whole runtime. See:
2019-04-16 14_02_54-mod ts (Working Tree) - deno_std - Visual Studio Code

Added on Silent mode the display of the currently running TEST:
2019-04-16 16_54_26-mod ts - deno_std - Visual Studio Code

@zekth zekth changed the title Pretty output + disable log handling [Testing] Pretty output + disable log handling Mar 29, 2019

Show resolved Hide resolved testing/mod.ts Outdated
Show resolved Hide resolved testing/mod.ts Outdated
Show resolved Hide resolved testing/mod.ts Outdated
Show resolved Hide resolved testing/mod.ts Outdated
Show resolved Hide resolved testing/mod.ts
@ry

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

I do not believe this issue is solvable. The correct solution is to just reduce printing to stdout during the tests.

@zekth

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2019

I do not believe this issue is solvable. The correct solution is to just reduce printing to stdout during the tests.

This is totally possible, here is the Jest way using --silent argument for example:
https://github.com/facebook/jest/blob/master/packages/jest-runner/src/runTest.ts#L137
https://github.com/facebook/jest/blob/master/packages/jest-console/src/NullConsole.ts

I think i can solve the problems raised by @bartlomieju . Or you prefer me to abort this?

@zekth zekth force-pushed the zekth:testing_prompt branch 2 times, most recently from 05e1c3b to 4ae64a4 Apr 9, 2019

@zekth

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2019

About the remaining output of prettier this is due to this:
https://github.com/denoland/deno_std/blob/master/prettier/main_test.ts#L15

And we can't do anything about it because it's another process running. But IMO the silent mode is a good addition for the tests.

Note: Silent mode is disabled in the CI.

@zekth zekth force-pushed the zekth:testing_prompt branch 3 times, most recently from f5f0417 to fefac6f Apr 16, 2019

@zekth

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2019

Last thing, for the silent mode we can make it totally silent by adding an eventlistenner on the Deno.stdout.write() and do a Deno.stdout.write(new TextEncoder().encode("\x1b[2K\r ")). This will go back to the start of the line and print a space instead of the targeted char.

@bartlomieju
Copy link
Contributor

left a comment

After some thought, I think it's worth considering to make a small helper:

function output(s: string) {
   Deno.stdout.writeSync(encoder.encode(s));
}

And using it for test purpose so usages of console are not mixed. Then you should be able to disable console globally, without restoring it for each test case. What do you think @zekth?

EDIT: Also, integration test is needed for this

Show resolved Hide resolved testing/mod.ts Outdated
Show resolved Hide resolved testing/mod.ts Outdated
Show resolved Hide resolved testing/mod.ts Outdated
Show resolved Hide resolved testing/mod.ts Outdated
@zekth

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2019

@bartlomieju agreed about the refactor of output. It would be simpler. Added the part of stdout with the 'Running' commit but didn't thought about refactoring it into a helper.

Also about testing how you want to test it as we can't stub the functions atm?

@zekth zekth changed the title [Testing] Pretty output + disable log handling [Testing] Pretty output + Silent mode Apr 17, 2019

@zekth

This comment has been minimized.

Copy link
Contributor Author

commented Apr 21, 2019

@bartlomieju about disablelog the only need is to prompt:

RUNNING test FOO

About this feature i still have a little problem because i can't attach an event on the stdout output, so if a test uses Deno.stdout.write instead of console.log it will still prompt and avoid the rewrite of the current line showing RUNNING test FOO.
2 possibilities:

  • Remove this and wait to the feature allowing to attach event to stdout and make possible to rewrite every unwanted print.
  • Keep it because using direct Deno.stdout is not recommended.
@zekth

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

Any opinion about it or we close it?

Show resolved Hide resolved testing/mod.ts Outdated
@bartlomieju

This comment has been minimized.

Copy link
Contributor

commented May 5, 2019

Any opinion about it or we close it?

This functionality is quite useful, IMHO we should land it. @ry?

@zekth zekth referenced this pull request May 6, 2019

Open

[Discussion] Testing #306

@ry

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

@zekth @bartlomieju It does look nice. I will defer to @piscisaureus on this.

@ry ry requested a review from piscisaureus May 6, 2019

@bartlomieju

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

CC @piscisaureus could you take a look?

@piscisaureus

This comment has been minimized.

Copy link
Contributor

commented May 30, 2019

I think this is generally a nice feature and it worked when I tested it, but I have two needs:

  • Is there a way I can run the tests with the console enabled, without hacking the test runner? Preferrably it would be easy to discover this feature, e.g. when I run deno run -A test.ts --help or something it should show up. I don't want to have to change the test runner when to debug tests.

  • Can you round down test times to 2 decimal places. Now it prints 3.2564999999995052ms. That's bonkers.

@zekth

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2019

  • Is there a way I can run the tests with the console enabled, without hacking the test runner? Preferrably it would be easy to discover this feature, e.g. when I run deno run -A test.ts --help or something it should show up. I don't want to have to change the test runner when to debug tests.

For example having:
deno run -A test.ts --quiet ?

  • Can you round down test times to 2 decimal places. Now it prints 3.2564999999995052ms. That's bonkers.

Sure will fix this.

@zekth zekth force-pushed the zekth:testing_prompt branch from 6b4735d to b86b208 May 31, 2019

@zekth

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2019

I've fixed the toFixed output and added parsing of Deno.args to check if --quiet is in.

Also just need to adjust for strict mode.
@bartlomieju i got this error:

Element implicitly has an 'any' type because type 'Console' has no index signature.

34     if (console[key] instanceof Function) {

console is consoleTypes.Console . May i add a @ts-ignore for those? or you have a better alternative?

@kitsonk any opinion?

@bartlomieju

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2019

@zekth sorry for delay. How about you write TestConsole class - it is dummy alternative for //js/console.ts:Console - basically replace printFunc. I think it'd be cleaner solution than overriding methods in for loop. WDYT?

@zekth

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2019

added // @ts-ignore but it's ugly yes. A solution is to add an index signature on : https://github.com/denoland/deno/blob/master/js/console.ts
like:

interface SignatureInterface {
    [key: string]: unknown
}

export class Console implements SignatureInterface {
  indentLevel: number;
  collapsedAt: number | null;
  [isConsoleInstance]: boolean = false;
....

Or write down the whole console interface.

@zekth zekth force-pushed the zekth:testing_prompt branch from 7239a13 to 11b3b37 Jun 14, 2019

@bartlomieju

This comment has been minimized.

Copy link
Contributor

commented Jun 16, 2019

@zekth can you try this patch:

diff --git a/testing/mod.ts b/testing/mod.ts
index ea090dc..f6db9c5 100644
--- a/testing/mod.ts
+++ b/testing/mod.ts
@@ -17,31 +17,44 @@ export interface TestDefinition {
   name: string;
 }
 
-// TODO: just construct single noop console
-
 // Replacement of the global `console` function to be in silent mode
-const noopConsole = function(): void {};
+const noop = function(): void {};
 
 // Save Object of the global `console` in case of silent mode
-let defaultConsole = {};
+type Console = typeof window.console;
+// ref https://console.spec.whatwg.org/#console-namespace
+// For historical web-compatibility reasons, the namespace object for
+// console must have as its [[Prototype]] an empty object, created as if
+// by ObjectCreate(%ObjectPrototype%), instead of %ObjectPrototype%.
+const disabledConsole = Object.create({}) as Console;
+Object.assign(disabledConsole, {
+  log: noop,
+  debug: noop,
+  info: noop,
+  dir: noop,
+  warn: noop,
+  error: noop,
+  assert: noop,
+  count: noop,
+  countReset: noop,
+  table: noop,
+  time: noop,
+  timeLog: noop,
+  timeEnd: noop,
+  group: noop,
+  groupCollapsed: noop,
+  groupEnd: noop,
+  clear: noop,
+});
+
+const originalConsole = window.console;
 
 function enableConsole(): void {
-  for (const key in defaultConsole) {
-    // @ts-ignore
-    console[key] = defaultConsole[key];
-  }
+  window.console = originalConsole;
 }
 
 function disableConsole(): void {
-  for (const key in console) {
-    // @ts-ignore
-    if (console[key] instanceof Function) {
-      // @ts-ignore
-      defaultConsole[key] = console[key];
-      // @ts-ignore
-      console[key] = noopConsole;
-    }
-  }
+  window.console = disabledConsole;
 }
 
 const encoder = new TextEncoder();

Seems to be working for me

@zekth

This comment has been minimized.

Copy link
Contributor Author

commented Jun 16, 2019

@piscisaureus you can review it now

@zekth

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2019

@ry any feedback?

@bartlomieju

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

Please land it, it's very useful for test runner

@ry

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

Yes - ok. Although I still have hesitations about how this is accomplished. It's certainly much better looking.

const encoder = new TextEncoder();
function print(txt: string, carriageReturn: boolean = true): void {
if (carriageReturn) {
txt += "\n";

This comment has been minimized.

Copy link
@ry

ry Jun 18, 2019

Contributor

This is not a carriage return character, this a new line character.

s/carriageReturn/newline/g

return `${(time / 1000).toFixed(2)}s`;
} else {
return `${time.toFixed(2)}ms`;
}

This comment has been minimized.

Copy link
@ry

ry Jun 18, 2019

Contributor

I'd prefer if you use a single time unit (ms)

console.groupEnd();
console.error(err.stack);
if (disableLog) {
print("\x1b[2K\r", false);

This comment has been minimized.

Copy link
@ry

ry Jun 18, 2019

Contributor

ANSI escape sequences are hard to read. Ideally you'd define them in a constant. Something like:

const CLEAR_LINE = "\x1b[2K";

(I don't actually know what that ANSI sequence means, I'm just giving an example)

This comment has been minimized.

Copy link
@zekth

zekth Jun 19, 2019

Author Contributor

This is used to erase the the current line: http://ascii-table.com/ansi-escape-sequences-vt-100.php

@bartlomieju

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

Yes - ok. Although I still have hesitations about how this is accomplished. It's certainly much better looking.

Agreed - this issue can be addressed better if Console is exported in Deno (ref denoland/deno#2521, although instead of exporting interface we could expose Console directly) - this would allow to capture full output of console via custom Console.printFunc and use it later. We'd still have to capture stdout but that can be addressed in separate PR.

@ry

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

Looks good modulo a couple nits...

@zekth

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2019

Done the review. Also added the comment about the ASCII table.

@ry

ry approved these changes Jun 19, 2019

Copy link
Contributor

left a comment

LGTM!

@ry ry merged commit d44a47a into denoland:master Jun 19, 2019

5 checks passed

denoland.deno_std Build #20190619.19 succeeded
Details
denoland.deno_std (Linux) Linux succeeded
Details
denoland.deno_std (Mac) Mac succeeded
Details
denoland.deno_std (Windows) Windows succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@zekth zekth deleted the zekth:testing_prompt branch Jun 19, 2019

@zekth

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2019

Note: At the moment the CI is not quiet. we may want to add the --quiet parameter?

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.