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

Added t.diag() method to print yaml metadata #99

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

andrewdeandrade
Copy link
Contributor

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

Raynos commented Aug 15, 2014

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

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

Ovid commented Aug 15, 2014

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 [email protected] 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
Copy link
Contributor Author

@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
Copy link
Collaborator

Raynos commented Aug 15, 2014

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

@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
Copy link
Collaborator

Raynos commented Aug 16, 2014

@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
Copy link
Collaborator

Raynos commented Mar 1, 2015

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

@vvo
Copy link

vvo commented Jun 9, 2015

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

@gabrielcsapo
Copy link
Contributor

👍 can we get this merged in?

@ljharb
Copy link
Collaborator

ljharb commented Jun 7, 2017

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

@gabrielcsapo
Copy link
Contributor

@isaacs are you maintaining this repo also?

@ljharb
Copy link
Collaborator

ljharb commented Jun 7, 2017

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

@gabrielcsapo
Copy link
Contributor

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

ljharb commented Jun 7, 2017

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

isaacs commented Jun 8, 2017

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

@gabrielcsapo
Copy link
Contributor

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

@isaacs
Copy link
Contributor

isaacs commented Jun 8, 2017

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

@gabrielcsapo
Copy link
Contributor

@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 ljharb added the semver-minor: new stuff Any additions. label Jan 28, 2019
@ljharb
Copy link
Collaborator

ljharb commented Jan 28, 2019

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

@ljharb ljharb added the needs "allow edits" PR needs "allow edits" checked label Jan 28, 2019
@ljharb ljharb marked this pull request as draft February 20, 2021 16:06
@Raynos
Copy link
Collaborator

Raynos commented Mar 4, 2024

Closing out due to lack of activity.

@Raynos Raynos closed this Mar 4, 2024
@ljharb
Copy link
Collaborator

ljharb commented Mar 4, 2024

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

@ljharb ljharb reopened this Mar 4, 2024
@ljharb ljharb force-pushed the master branch 3 times, most recently from c16dde3 to 2ad86d4 Compare June 16, 2024 06:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs "allow edits" PR needs "allow edits" checked semver-minor: new stuff Any additions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Could use a diag() Method
7 participants