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

WIP test: improve testing tools #946

Closed
wants to merge 3 commits into from

Conversation

kt3k
Copy link
Member

@kt3k kt3k commented Oct 9, 2018

This resolves some TODOs in the testing library.

  • Shows the ok / FAILED message in the same line as the name of the test case.
  • Counts the number of filtered tests.

Waiting for #948 to be resolved first.

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.

Hi @kt3k thank you but I don’t want to land this one -

#923 already addresses this counters (which I will land shortly) and this doesn’t properly solve the name-printing problem.

try {
await fn();
console.log("test", name, "...", green_ok());
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 like the test name to be displayed before the test is run, so if it crashes there is still evidence of what caused it. Here tha name is printed after “fn()”

Copy link
Contributor

Choose a reason for hiding this comment

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

Could this instead first write to stdout instead of console.log (so as to avoid the newline)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried deno.stdout.write(new TextEncoder().encode('message')) to print the message without newilne, but it didn't print anything...

Copy link
Member

Choose a reason for hiding this comment

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

@kt3k That is likely because TTYs are line buffered by default. To do this we probably need call sync(2) or set the TTY in unbuffered mode.

Maybe @piscisaureus remembers how we did this in Node...

Copy link
Member

@ry ry Oct 9, 2018

Choose a reason for hiding this comment

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

Upon further investigation, we are using tokio-fs's stdout, which writes to std::io::Stdout, which has a flush method (suggesting it is line buffered). If we were issuing a write syscall directly, this wouldn't happen - so my comment about needing to set the TTY into unbuffered mode was wrong.
We should probably expose that flush method as deno.stdout.flush()...

Copy link
Member

Choose a reason for hiding this comment

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

Rust's stdout is always line buffered:
https://github.com/rust-lang/rust/blob/e1643a8968753226dab7ab3c9fee826f097550f2/src/libstd/io/stdio.rs#L344-L349
We are not so happy with this behavior and likely want an always unbuffered stdout, as in Node. Therefore to support his behavior we will need to refactor the stdout/stderr resources to use a raw file descriptor like this:

use std::os::unix::io::FromRawFd;
let stdout = File::from_raw_fd(1);

Copy link
Member

Choose a reason for hiding this comment

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

I've opened an issue for this: #948

@kt3k
Copy link
Member Author

kt3k commented Oct 9, 2018

@ry Sorry for the duplication. I'll revert that part.

@kt3k kt3k changed the title test: improve testing tools WIP test: improve testing tools Oct 10, 2018
@kt3k
Copy link
Member Author

kt3k commented Oct 24, 2018

I will submit this again when #948 solved or when other solution available

@kt3k kt3k closed this Oct 24, 2018
@kt3k kt3k deleted the feature/improve-testing branch January 16, 2019 13:08
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.

3 participants