Skip to content
This repository has been archived by the owner on Dec 29, 2021. It is now read-only.

Testing binary output #80

Closed
vitiral opened this issue Jan 10, 2018 · 12 comments
Closed

Testing binary output #80

vitiral opened this issue Jan 10, 2018 · 12 comments
Labels

Comments

@vitiral
Copy link

vitiral commented Jan 10, 2018

Hello!

I just released the termstyle crate which makes it easier to build and test terminal applications that use color and tables (horray!).

I was hoping to use it with assert_cli to test my crate, however the is() method seems to requre that the input can be coerced into a String, which terminal output with color certainly cannot.

Separate question: is there any interest in combining forces here? We could recommend termstyle as a possible crate to build your cli with (probably behind a feature flag) and we could add a has_els() method (or something) to make it more convinient to build assertions.

Edit: See this comment. Most ANSI escape codes are acually valid UTF8!

@epage epage added the bug label Jan 10, 2018
@epage
Copy link
Collaborator

epage commented Jan 10, 2018

I was hoping to use it with assert_cli to test my crate, however the is() method seems to require that the input can be coerced into a String, which terminal output with color certainly cannot.

I think its fully reasonable that we support this. We'll just need to make sure we work to keep the API simple for those that don't need this.

Separate question: is there any interest in combining forces here? We could recommend termstyle as a possible crate to build your cli with (probably behind a feature flag)

I think noting other relevant crates is really valuable for the ecosystem. Right now, assert_cli advertises crates crates to help test command line tools and clap advertises assert_cli for testing programs made with it.

My initial reaction is to keep this directionality.

  • This aligns with how developers will solve problems. They think "I need feature X" (like output formatting) and find the crate for it. They then think "I need to test my command line" and find the crate for it. Giving the developer "oh, you're testing a command line, you'll probably also want to use these other crates to test it" makes sense. A developer is unlikely to look to the testing crate to help them solve non-testing problems
  • So that at least leaves endorsing crates in the wider sphere for discoverability. The challenge here is that there are a lot of different needs that lead to a lot of different policies that leads to a lot of different crates. It'd be hard for us to take on deciding what should be made more discoverable. That seems like that should be more organic (actual usages of your crate, blog posts of people talking about what they've made, crates.io showing popular crates).

If we find a good way for our crates to interoperate, then I think it'd make sense for your crate to mention assert_cli and maybe even include examples.

and we could add a has_els() method (or something) to make it more convinient to build assertions.

We'll probably want to brainstorm what color/format assertions make sense and what is the best way to expose them in the API. One approach is just to make helper functions that can easily be boxed into Fns (#55). If we directly added color testing support, I think we'd want to depend on a lower level crate for that to minimize what bloat we'd adding to clients.

@killercup
Copy link
Collaborator

Hi! @epage already answered very thoughtfully!

I just wanted to confirm that the only reason we take Strings everywhere is probably because it was the easiest thing to do when I wrote this code initially and we never changed it. I'd be fine with switching to Into<OsString> if we keep the API the same.

Another note: assert_cli uses regular sub-processes and captures their std{out,err} in a buffer. Usually, these buffers to not register as a tty that supports color.

@vitiral
Copy link
Author

vitiral commented Jan 10, 2018

I think we need Vec<u8>, not OsString. OsString is for representing any OS's path. I don't think you can use escape codes (i.e. color, bold, etc) in file names!

I agree with everything else said. It would be great to get this feature!

@epage
Copy link
Collaborator

epage commented Jan 10, 2018

Huh, I assumed Command was using OsString, but you are right, it is using Vec

@killercup
Copy link
Collaborator

killercup commented Jan 10, 2018 via email

@vitiral
Copy link
Author

vitiral commented Jan 10, 2018

My crate currently "supports" windows terminal colors by ignoring all styles. Accoridng to @BurntSushi "you don't" test windows terminal colors anyway

I would like to send the right signals (or whatever is required, I haven't looked into it yet) to support windows colors for the end users though, even if it isn't testable. I think it should be pretty simple too hook in burntsushi's library to do so.

In order to diff better, I recommend:

  • attempt to convert using String::from_utf8, if it converts then just use that!
  • If it doesn't convert, convert it into a string through it's "repr bytes" using something like the following, and then do a pretty-diff against that:
/// Helper function to make tests easier for others.
///
/// Represent a series of bytes with proper escape codes for
/// copy/pasting into rust surounded by `b"..."`.
pub fn write_repr<W: io::Write>(w: &mut W, bytes: &[u8]) -> io::Result<()> {
    for b in bytes {
        match *b {
            b'\t' => write!(w, r"\t")?,
            b'\n' => write!(w, r"\n")?,
            b'\r' => write!(w, r"\r")?,
            b'\\' => write!(w, r"\\")?,
            32...126 => write!(w, "{}", *b as char)?, // visible ASCII
            _ => write!(w, r"\x{:0>2x}", b)?,
        }
    }
    Ok(())
}

#[test]
fn sanity_repr() {
    let r = |b| {
        let mut w: Vec<u8> = Vec::new();
        write_repr(&mut w, b).unwrap();
        String::from_utf8(w).unwrap()
    };
    assert_eq!(r"foo", r(b"foo"));
    assert_eq!(r"\\", r(b"\\"));
    assert_eq!(r"\x8a", r(b"\x8a"));
    assert_eq!(r"\x8a", r(&[0x8a]));
}

This makes it much easier to see what the differences are.

Honestly though, if OsString can actually accept arbitrary bytes then I would say just use that! I thought it did some kind of checking though, in which case I would say don't use it.

Edit: you said it "guarantees WTF-8 encoding" -- so that would not be arbitrary and so wouldn't work.

@vitiral
Copy link
Author

vitiral commented Jan 10, 2018

Let me also say a little about what I intend to do when I unify these libraries. This feels like the same rough goal as assert_cli so I thought you might be interested in joining forces (with a feature flag) instead of a separate crate.

I would like to be able to specify the assertions to do for any given project through a simple yaml file. So let's say that I had a project

project/
  foo.txt
  assert_cli.yaml

where foo.txt contains:

This is foo
It has multiple lines

You could then write assert_cli.yaml:

- commands:
  - cmd: echo foo.txt
  - expect: # maybe change to "is"?
    - "This is foo\n"
    - "It has multiple lines\n"
  - prints: # support for other assert_cli methods
    - "This is foo"

It doesn't look that cool at first glance, until you realize that expect field is just a Vec<termstyle::El>. So you can specify style and formatting as well! So you could have written something like:

- {t: "this line is red\n", c: red}
- ["this line has ", {t: "mixed styles ", b: true}, {t: "like bold and blue\n", c: blue}]

Edit: forgot to add the API

assert_cli would then have a method assert_project<P: AsRef<Path>>(root: P) where it would perform the requested commands and assert the expected values.

Edits: I added prints in the yaml file.

@vitiral
Copy link
Author

vitiral commented Jan 10, 2018

If you are interested I can open a separate ticket. If not, I totally understand wanting to keep things simple. I can start my own project!

I wish I could call it assert_cli though 😦

@killercup
Copy link
Collaborator

killercup commented Jan 10, 2018 via email

@vitiral
Copy link
Author

vitiral commented Jan 10, 2018

Sounds good! I was also wary of including so much extra functionality in a crate like this.

Do note that termstyle doesn't depend on yaml -- it can use any serde crate that exports a from_str function. But I take your point.

As soon as this bug get's resolved, I'll get working on that 😄

@epage
Copy link
Collaborator

epage commented Jan 11, 2018

@killercup that is also what I remember us deciding to do.

@vitiral
Copy link
Author

vitiral commented Jan 18, 2018

@BurntSushi made an excellent point in a recent reddit thread that most ANSI escape codes are actually valid unicode!

So while this is still an issue (since technically the terminal can output non-UTF8 bytes) it can probably be worked around by most people.

Just thought I would let anyone know that they probably can use assert_cli to test a terminal application with color today!

I'm changing the title to reflect the actual state of things.

@vitiral vitiral changed the title Testing with color (ansi escape codes) Testing binary output Jan 18, 2018
epage added a commit to epage/assert_cli that referenced this issue Feb 3, 2018
If you pass a `&str` into the predicates, the programs output is assumed
to be UTF-8 and will be converted accordingly.

If you pass `&[u8]` into the predicates, the program output is treated
as raw binary.

Fixes assert-rs#80
@epage epage closed this as completed in 230ecac Feb 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants