Added t.diag() method to print yaml metadata

[andrewdeandrade]

This pull requests implements a t.diag() method that takes JSON and can print out the json as a YAML block. FWIW this adds jeremyfa/yaml.js as a new dependency, but that dependency works in browsers.

This would close #91 but before we do, I think it may be worth discussing this a bit.

The TAP specification states:

After a test line a block of lines starting with '---' and ending with '...' will 
be interpreted as an inline YAML document providing extended 
diagnostic information about the preceding test.

This seems to imply that extended diagnostic information should be attached to the preceding test. Having a standalone diag() method in a way implies that the diagnostic information to be printed is standalone information.

Now, the way I see it, there are three options here:

• Keep this pull request the way it is. The downside is that the diagnostic information is not "attached" to the assertion in question. The upside is that this frees someone up to print that diagnostic information later, but before the next assertion is executed. The reasons for this may be that the diagnostic information is not available at the time the assertion is executed.
• Permit someone to add diagnostic information as a third argument, so that you could do t.ok(true, 'Some message', someDiagnosticJSONObj). However, his approach would require quite a bit of reworking of tape since tape uses the third argument internally as an extra argument. This means that you'd have to do some whitelisting of the extra property object when you print it out.
• Allow both approaches.

Lastly, should we allow people to pass in YAML content directly into the diag method? AFAICT, the only thing necessary to do this is to check if the argument to diag() is a string or a plain old javascript object, and handle it accordingly.

Anyone have any strong thoughts here?

(FWIW I also looked into using YAML.stringify in the errorYAML function, but that didn't really work because it would YAMLize the actual and expected properties, even if they were JSON objects.)

[Raynos]

The diag implementation looks good.

It's a useful method, explain() would also be really useful, i'd use that one more.

Thanks for the very detailed pull request :)

[andrewdeandrade]

@Raynos Implementing explain() should be trivial. There is just one detail to resolve first: How exactly should one put tape into verbose mode when running in the browser? On the command line this can be trivially accomplished with a --verbose CLI flag. In a server-side process you can use a command line flag when starting the process or use ENV variables. With a browser side test, this isn't so easy since you may not have any real control over how the runtime gets executed/setup or what global context is available at runtime. The best option I can think of is setting a global flag that is set by your build system when in concatenates your JS files when dynamically generating JS scripts.

Also, should explain() be a binary on/off feature, or should it support some sort of log levels feature. e.g. only show me logging information of type performance or with importance above a level of 40

@Ovid any thoughts on diagnostic output levels versus a simple on/off switch?

[Ovid]

I don't think explain() needs anything akin to log levels primarily because I use it when I'm writing new tests and implementing a feature. When I do that, I want to see those explain messages — all of them. Thus, for me it really is a boolean on/off.

That being said, maybe someone else has a different use case I'm unaware. I teach a lot of people how to test and I haven't seen a different use case, but it doesn't mean it doesn't exist.
 
Best,

Ovid

IT consulting, training, international recruiting
       http://www.allaroundtheworld.fr/.
Buy my book! - http://bit.ly/beginning_perl
Live and work overseas - http://www.overseas-exile.com/

On Friday, 15 August 2014, 12:17, Andrew de Andrade notifications@github.com wrote:

@Raynos Implementing explain() should be trivial. There is just one detail to resolve first: How exactly should one put tape into verbose mode when running in the browser? On the command line this can be trivially accomplished with a --verbose CLI flag. In a server-side process you can use a command line flag when starting the process or use ENV variables. With a browser side test, this isn't so easy since you may not have any real control over how the runtime gets executed/setup or what global context is available at runtime.
Also, should explain() be a binary on/off feature, or should it support some sort of log levels feature. e.g. only show me logging information of type performance or with importance above a level of 40
@Ovid any thoughts on diagnostic output levels versus a simple on/off switch?
—
Reply to this email directly or view it on GitHub.

[andrewdeandrade]

@Ovid cool. That's what I figured, but since I haven't had access to an explain() method, it's best to hear from someone who has experience knowing how it's used.

@Raynos Any thoughts on how the explain flag should be turned on?

[Raynos]

@malandrew

• TAP_EXPLAIN=1 as env variable.
• --explain as command line argument.
• ?explain=true as query string in browser
• window.TAP_EXPLAIN=1 as global in browser (approximation of env variable)
• #explain=true as hash section in browser
  -require('tape').explain = true as code driven togglable for node & browser.

I personally like TAP_EXPLAIN in env variable, window and ?explain=true in query string (browser).

Probably supporting require('tape').explain = true is not a bad idea.

[andrewdeandrade]

@Raynos you, sir, are an officer and a gentleman. thank you. I'll open up a new pull request for explain(), but it will be dependent on diag() being merged in. I'll add each of those in separate commits so they can be cherry picked if there is one we decide we don't want.

Personally, my two favorites are your two favorites as well. The window.TAP_EXPLAIN would also need to work with global.TAP_EXPLAIN so that it works in node and in the browser. The --explain command line argument really is a function of the test runner and is out of scope for substack/tape, but not out of scope for isaacs/node-tap.

Is there any reason to support both the query string and the hash section? Seems like only one should be implemented for simplicity and that querystring is a better choice since it's semantically congruent with the intent of querystrings (whereas hashes were originally meant to be for anchors and AFAIK either don't support an X=Y syntax or at least was never meant to).

Agree that the require('tape').explain = true is not a bad idea, since that leaves someone the option for only turning on explain inline for a few assertions and then disabling it again. Should it just be a boolean or implemented as tape.enableExplain() and tape.disableExplain()?

For the ENV vars, is there a browserify "transform"/"plugin" that supports receiving an array of ENV variables that should be included in the final browserified script as a global? i.e. $ browserify --env=TAP_EXPLAIN,DEBUG ./my-file.js > bundle.js

[Raynos]

@malandrew I prefer a tape.explain boolean because it doesnt add more methods to the interface and it's kind of a hackable implementation detail.

i.e. tape.explain = process.env.TAP_EXPLAIN || global.TAP_EXPLAIN || ... if you implement the explain toggle like that and then use if (tape.explain) in the test.explain() method, you get the feature that people can toggle it to something different without env variables / globals / uri strings.

I think doing both query & hash in the uri is overkill, query string is fine.

There is an https://github.com/hughsk/envify transform that inlines all process.env.foo

[Raynos]

Do we still want this ? If so we can #shipit

[vvo]

@Raynos Yes! tap-parser supports diag so it would be good

[gabrielcsapo]

👍 can we get this merged in?

[ljharb]

It needs a rebase and conflicts resolved, or, the "allow edits" checkbox checked.

[gabrielcsapo]

@isaacs are you maintaining this repo also?

[ljharb]

@gabrielcsapo please don't ping arbitrary people unnecessarily; this PR can't be merged until the OP, @malandrew, updates it.

[gabrielcsapo]

I was going to rebase the commit if @malandrew didn't respond. I was asking the question to Isaacs if this is being maintained any longer or if I should just move to tap. @ljharb

[ljharb]

Yes, of course tape is being maintained, by multiple people.

I'd prefer the original poster be the one to do it, so we can modify this PR in-place instead of creating another one and polluting the git log.

[isaacs]

I am not maintaining tape. All the tap-related things I do are under the tapjs github org.

[gabrielcsapo]

@isaacs does that mean node-tape is being deprecated in favor of tapjs?

[isaacs]

@gabrielcsapo No. Tape and node-tap are separate projects. Always have been.

[gabrielcsapo]

@isaacs okay, but the output is based on the tap standard version 13 currently right? I was looking to see if tape would be updated to adhere to that spec.

[ljharb]

@andrewdeandrade can you please check the "allow edits" checkbox on the RHS of the PR?

[Raynos]

Closing out due to lack of activity.

[ljharb]

I prefer to keep inactive PRs open, in case the author returns.