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
Merged

[Testing] Pretty output + Silent mode #314

merged 12 commits into from
Jun 19, 2019

Conversation

zekth
Copy link
Contributor

@zekth zekth 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
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
@ry
Copy link
Member

ry 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
Copy link
Contributor Author

zekth 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
Copy link
Contributor Author

zekth 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
Copy link
Contributor Author

zekth 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.

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.

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

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
@zekth
Copy link
Contributor Author

zekth 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
Copy link
Contributor Author

zekth 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
Copy link
Contributor Author

zekth commented Apr 30, 2019

Any opinion about it or we close it?

testing/mod.ts Outdated Show resolved Hide resolved
@bartlomieju
Copy link
Member

Any opinion about it or we close it?

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

@zekth zekth mentioned this pull request May 6, 2019
@ry
Copy link
Member

ry 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 15:02
@bartlomieju
Copy link
Member

CC @piscisaureus could you take a look?

@piscisaureus
Copy link
Member

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
Copy link
Contributor Author

zekth 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
Copy link
Contributor Author

zekth 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
Copy link
Member

@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
Copy link
Contributor Author

zekth 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.

@bartlomieju
Copy link
Member

@zekth could you merge master into this branch?

@bartlomieju
Copy link
Member

bartlomieju 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
Copy link
Contributor Author

zekth commented Jun 16, 2019

@piscisaureus you can review it now

@zekth
Copy link
Contributor Author

zekth commented Jun 18, 2019

@ry any feedback?

@bartlomieju
Copy link
Member

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

@ry
Copy link
Member

ry 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";
Copy link
Member

Choose a reason for hiding this comment

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

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

s/carriageReturn/newline/g

testing/mod.ts Outdated
return `${(time / 1000).toFixed(2)}s`;
} else {
return `${time.toFixed(2)}ms`;
}
Copy link
Member

Choose a reason for hiding this comment

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

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

testing/mod.ts Outdated
console.groupEnd();
console.error(err.stack);
if (disableLog) {
print("\x1b[2K\r", false);
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@bartlomieju
Copy link
Member

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
Copy link
Member

ry commented Jun 18, 2019

Looks good modulo a couple nits...

@zekth
Copy link
Contributor Author

zekth commented Jun 19, 2019

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

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

LGTM!

@ry ry merged commit d44a47a into denoland:master Jun 19, 2019
@zekth zekth deleted the testing_prompt branch June 19, 2019 12:44
@zekth
Copy link
Contributor Author

zekth commented Jun 19, 2019

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

ry pushed a commit to ry/deno that referenced this pull request Oct 9, 2019
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.

5 participants