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

Less API using power-assert #46

Closed
wants to merge 13 commits into from
Closed

Less API using power-assert #46

wants to merge 13 commits into from

Conversation

uiur
Copy link
Contributor

@uiur uiur commented Sep 9, 2015

I just want to share my demo, so feel free to close this PR. Just ignore it if you don't like it.

Added t.ok(value, [message]) using power-assert. It shows descriptive assertion message by parsing the JS expression.

example

test((t) => {
  var a = { name: 'foo' }

  t.ok(a.name === 'bar')

  t.end()
})

shows a message like this.

> ava

   [anonymous]   # test.js:26
  t.ok(a.name === 'bar')
       | |    |         
       | |    false     
       | "foo"          
       Object{name:"foo"}

  --- [string] 'bar'
  +++ [string] a.name
  @@ -1,3 +1,3 @@
  -bar

apis

t.true(true)
t.is('foo', 'foo');
t.not('foo', 'bar');
t.same({a: 'a'}, {a: 'a'});
t.regexTest(/^abc$/, 'abc');

with power-assert:

var deepEqual = require('deep-equal');

t.ok(true);
t.ok('foo' === 'foo');
t.ok('foo' !== 'bar');
t.ok(deepEqual({a: 'a'}, {a: 'a'}));
t.ok(/^abc$/.test('ab'));

more example

   [anonymous]   # test.js:44

  t.ok(deepEqual(a, b))
       |         |  |  
       |         |  ["a","a"]
       false     ["a","b"]

   [anonymous]   # test.js:50

  t.ok(/^abc$/.test(`ab${ s }`))
               |    |     |     
               |    "aba" "a"   
               false            

I know custom assertion is supported on ava and it worked pretty well.
But I wanted to use t.plan() and that assertion function with ava's minimal interface.

@Qix-
Copy link
Contributor

Qix- commented Sep 9, 2015

It'd be nice if this translated into some way to hook into Ava to allow it to use any arbitrary assertion library, including support for t.plan().


Side note, power-assert looks badass.

@uiur uiur closed this Sep 9, 2015
@Qix-
Copy link
Contributor

Qix- commented Sep 9, 2015

@uiureo why did you close? :o

@uiur
Copy link
Contributor Author

uiur commented Sep 10, 2015

Because I don't intend this PR to be merged. I don't want the opened state to bother you guys.

It'd be nice if this translated into some way to hook into Ava to allow it to use any arbitrary assertion library, including support for t.plan().

That's the very thing I want.
I'm looking into the code and I would try if I come up with some way.

Related: #25

@Qix-
Copy link
Contributor

Qix- commented Sep 10, 2015

👍 sounds good

@sindresorhus
Copy link
Member

I'd like to keep this open for a while so people can see it. I think power-assert is really cool and I actually considered including it in AVA early on. The reasons I didn't were the compile step (which is not a problem now that we include Babel by default) and the result, while being very informative, is a bit noisy. foo === bar is a lot clearer and faster to grasp. Is there any way to simplify the output?

@sindresorhus sindresorhus reopened this Sep 10, 2015
@uiur
Copy link
Contributor Author

uiur commented Sep 10, 2015

the result, while being very informative, is a bit noisy. foo === bar is a lot clearer and faster to grasp. Is there any way to simplify the output?

I agree its output is a little bit noisy. It seems you can write custom output formatter in power-assert.

https://github.com/power-assert-js/power-assert#optionsoutput
https://github.com/power-assert-js/power-assert-formatter

FYI @twada

@twada
Copy link
Contributor

twada commented Sep 10, 2015

Hi, I'm creator of power-assert.
Thank you for mentioning me @uiureo.
I'm very interested in AVA now.

power-assert output consists of 4 parts, rendered by 4 renderers (file, assertion, diagram, and binary-expression).
And you can omit each part from output through customization API.

For example, output below

  # test.js:26
  t.ok(a.name === 'bar')
       | |    |         
       | |    false     
       | "foo"          
       Object{name:"foo"}
  --- [string] 'bar'
  +++ [string] a.name
  @@ -1,3 +1,3 @@
  -bar

file renderer produces,

   # test.js:26

assertion renderer produces,

  t.ok(a.name === 'bar')

diagram renderer produces,

       | |    |         
       | |    false     
       | "foo"          
       Object{name:"foo"}

and binary-expression renderer produces

  --- [string] 'bar'
  +++ [string] a.name
  @@ -1,3 +1,3 @@
  -bar

To disable verbose graph, remove diagram renderer from output.renderers by using customize method. For example, this configuration

var assert = require('power-assert').customize({
    output: {
        renderers: [
            './built-in/file',
            './built-in/assertion',
            './built-in/binary-expression'
        ]
    }
});

produces output as below.

  # test.js:26
  t.ok(a.name === 'bar')

  --- [string] 'bar'
  +++ [string] a.name
  @@ -1,3 +1,3 @@
  -bar

And of course, I want to hear your request, and create custom formatter for AVA!

FYI:
You can also create and add custom Renderer Class to configuration options.

@Qix-
Copy link
Contributor

Qix- commented Sep 10, 2015

To be honest, the binary renderer is the part that seems noisy to me. The diagram is what makes it super useful.

Thanks for the information @twada :D Very tempting to switch from should. I'll play with it a bit more.

@sindresorhus
Copy link
Member

Instead of:

  # test.js:26
  t.ok(a.name === 'bar')
       | |    |         
       | |    false     
       | "foo"          
       Object{name:"foo"}
  --- [string] 'bar'
  +++ [string] a.name
  @@ -1,3 +1,3 @@
  -bar

I think I would want something like:

  # test.js:26
  a.name === 'bar'
     |
     |
   "foo"          

@twada
Copy link
Contributor

twada commented Sep 11, 2015

Hi @Qix-

To be honest, the binary renderer is the part that seems noisy to me. The diagram is what makes it super useful.

Understood. You can disable binary-expression renderer by removing './built-in/binary-expression' from output.renderers

var assert = require('power-assert').customize({
    output: {
        renderers: [
            './built-in/file',
            './built-in/assertion',
            './built-in/diagram'
        ]
    }
});

You'll see output like

  # test.js:26
  t.ok(a.name === 'bar')
       | |    |         
       | |    false     
       | "foo"          
       Object{name:"foo"}

Very tempting to switch from should. I'll play with it a bit more.

I'm glad to hear that! 😄

@twada
Copy link
Contributor

twada commented Sep 11, 2015

Hi @sindresorhus, thank you for your request.
I'll create succinct output renderer for AVA!

@sindresorhus
Copy link
Member

@twada Any updates? Would really like to use power-assert to improve AVAs assert output.

@twada
Copy link
Contributor

twada commented Oct 11, 2015

@sindresorhus I've extracted power-assert-renderers module out and now creating less-noisy succinct-diagram renderer for AVA.

I would like you to see succinct-diagram-test.js.
(You'll see assert or t.ok calls are still remaining in output, but it's a bit difficult to remove for now)

If you like it, I'm going to integrate it into this pull-req and also power-assert master.

@sindresorhus
Copy link
Member

@twada Yes, looks very good :) I'm fine with this for now. Would be awesome if you would integrate it into this PR and fix the merge conflict.

@kevva
Copy link
Contributor

kevva commented Oct 20, 2015

Really exciting stuff.

@uiur
Copy link
Contributor Author

uiur commented Oct 20, 2015

I did quick hack to use the succinct renderer and fixed the merge conflict.

Now the output looks like this:

❯ npm test    

> ava

  ✖ [anonymous]   
  t.ok(a.name === 'bar')
         |              
         "foo"          


  1 test failed

  1. [anonymous]
  AssertionError:   
  t.ok(a.name === 'bar')
         |              
         "foo"          
    at decoratedAssert (/Users/zat/.ghq/github.com/sindresorhus/ava/node_modules/power-assert/node_modules/empower/lib/decorate.js:42:30)
    at Function.powerAssert (/Users/zat/.ghq/github.com/sindresorhus/ava/node_modules/power-assert/node_modules/empower/index.js:58:32)
    at Test.ok (/Users/zat/.ghq/github.com/sindresorhus/ava/lib/test.js:85:15)
    at Test.fn (/Users/zat/.ghq/github.com/uiureo/ava-power-assert-example/test.js:26:5)
    at Test.<anonymous> (/Users/zat/.ghq/github.com/sindresorhus/ava/lib/test.js:124:19)
    at tryCatcher (/Users/zat/.ghq/github.com/sindresorhus/ava/node_modules/bluebird/js/main/util.js:26:23)
    at Promise._resolveFromResolver (/Users/zat/.ghq/github.com/sindresorhus/ava/node_modules/bluebird/js/main/promise.js:480:31)
    at new Promise (/Users/zat/.ghq/github.com/sindresorhus/ava/node_modules/bluebird/js/main/promise.js:70:37)
    at Test.run (/Users/zat/.ghq/github.com/sindresorhus/ava/lib/test.js:110:9)
    at tryCatcher (/Users/zat/.ghq/github.com/sindresorhus/ava/node_modules/bluebird/js/main/util.js:26:23)
    at ReductionPromiseArray._promiseFulfilled (/Users/zat/.ghq/github.com/sindresorhus/ava/node_modules/bluebird/js/main/reduce.js:105:38)
    at ReductionPromiseArray.init [as _init$] (/Users/zat/.ghq/github.com/sindresorhus/ava/node_modules/bluebird/js/main/promise_array.js:92:18)
    at ReductionPromiseArray.init (/Users/zat/.ghq/github.com/sindresorhus/ava/node_modules/bluebird/js/main/reduce.js:42:10)
    at Async._drainQueue (/Users/zat/.ghq/github.com/sindresorhus/ava/node_modules/bluebird/js/main/async.js:128:12)
    at Async._drainQueues (/Users/zat/.ghq/github.com/sindresorhus/ava/node_modules/bluebird/js/main/async.js:133:10)
    at Async.drainQueues (/Users/zat/.ghq/github.com/sindresorhus/ava/node_modules/bluebird/js/main/async.js:15:14)

npm ERR! Test failed.  See above for more details.

@@ -15,7 +17,15 @@ try {

require(localBabel)(options);
} catch (err) {
require('babel-core/register')(options);
require('babel-core/register')(assign({}, options, {
Copy link
Member

Choose a reason for hiding this comment

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

The options here need to be put in a variable and used in line 18 too.

@twada
Copy link
Contributor

twada commented Oct 20, 2015

@uiureo Thanks for responding so quickly, nice to see this being merged!

@@ -70,6 +78,19 @@ Object.keys(assert).forEach(function (el) {
};
});

Test.prototype.ok = function () {
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be in the assert.js file.

@sindresorhus
Copy link
Member

@twada Can you review?

@twada
Copy link
Contributor

twada commented Oct 20, 2015

@sindresorhus modifyMessageOnRethrow option indicates whether to modify AssertionError message on (re)throwing AssersionError or not.

Should we enable it?

You are right. Should be true in this case.

(power-assert overwrites empower's default for ease of use)

@uiur
Copy link
Contributor Author

uiur commented Oct 21, 2015

I've tried to add patterns like this:

    plugins: [
        createEspowerPlugin(require('babel-core'), {
            patterns: [
                't.ok(value, [message])',
                't.notOk(value, [message])',
                't.true(value, [message])',
                't.false(value, [message])',
                't.is(value, expected, [message])',
                't.not(value, expected, [message])',
                't.same(value, expected, [message])',
                't.notSame(value, expected, [message])',
                't.regexTest(regex, contents, [message])'
            ]
        })
    ]
test((t) => {
  var a = 'foo'
  t.is(a, 'bar')
  t.end()
})

But I got weird output like this.

  ✖ [anonymous] { powerAssertContext: { value: 'foo', events: [ [Object] ] },
  source: { content: 't.is(a, \'bar\')', filepath: 'test.js', line === 'bar'

t.ok() works correctly.

@twada
Copy link
Contributor

twada commented Oct 21, 2015

@uiureo Thank you for reporting.
In this case, intrumentation patterns in lib/babel.js should be the same.
So please configure lib/babel.js and try again.

function enhanceAssert(assert) {
    empower(assert,
        powerAssertFormatter({
            renderers: [
                powerAssertRenderers.AssertionRenderer,
                powerAssertRenderers.SuccinctRenderer
            ]
        }),
        {
            destructive: true,
            modifyMessageOnRethrow: true,
            saveContextOnRethrow: false,
            patterns: [
                't.ok(value, [message])',
                't.notOk(value, [message])',
                't.true(value, [message])',
                't.false(value, [message])',
                't.is(value, expected, [message])',
                't.not(value, expected, [message])',
                't.same(value, expected, [message])',
                't.notSame(value, expected, [message])',
                't.regexTest(regex, contents, [message])'
            ]
        }
    );
}

@uiur
Copy link
Contributor Author

uiur commented Oct 21, 2015

@twada
Ah, I missed the point.
It works right. Thanks for help.


execCli('fixture/power-assert.js', function (err, stdout, stderr) {
t.ok(err);
t.true(stderr.indexOf('t.ok(a === \'bar\')\n') !== -1);
Copy link
Member

Choose a reason for hiding this comment

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

How is this testing that it's working? It's only the normals output.

And should be:

t.not(stderr.indexOf('t.ok(a === \'bar\')'), -1);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without power-assert, the output should be false == true because this assert is t.ok().
I thought probably it renders t.ok(a === 'bar') only when power-assert works.

test(t => {
    const a = 'foo';

    t.ok(a === 'bar');
    t.end();
});

Should I test this renders the diagram output somehow?

  t.ok(a === 'bar')
       |           
       "foo"       

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a test that it renders diagram output by regexp.
Let me know if you want some changes.

@uiur
Copy link
Contributor Author

uiur commented Oct 24, 2015

@sindresorhus I've done some tweaks. Can you review again?

@sindresorhus
Copy link
Member

Landed. Woop woop. Great work @uiureo. And thanks for reviewing @twada :)

@uiur
Copy link
Contributor Author

uiur commented Oct 24, 2015

Thanks a lot @sindresorhus @twada. I couldn't do this without your great help :)

@twada
Copy link
Contributor

twada commented Oct 24, 2015

@sindresorhus Wow I'm very glad to see this pull-req is getting merged!! 🎉
Thank you so much @uiureo for your great work!

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