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

Switch testing to tape #16

Closed
wants to merge 1 commit into from
Closed

Conversation

denis-sokolov
Copy link
Contributor

This is primarily to support a future addition of airtap which relies on tape and from limited testing does not work with tap.

Both projects seem to be supported and popular, the API similar, it seems the primary way of choosing between the two is a matter of taste and required advanced functionality. (example)

Our existing suite works fine with both, we do not rely on any advanced functionality.

Checklist

@denis-sokolov denis-sokolov mentioned this pull request Jan 18, 2021
4 tasks
This is primarily to support a future addition of airtap which relies on tape.
@jsumners
Copy link
Member

Why is tap not usable?

Copy link
Member

@RafaelGSS RafaelGSS left a comment

Choose a reason for hiding this comment

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

tape doesn't have Istanbul coverage builtin, at some point I think that should be good to have coverage in our CI. nyc should be installed manually to support the coverage. Also, I don't see why tap is not usable.

@denis-sokolov
Copy link
Contributor Author

airtap uses tape. It’s in their demo, in their dependencies. I am unfamiliar how to integrate airtap with tap, if I run as is, it crashes.

airtap comes with nyc and coverage already, so perhaps that will resolve your concerns. Or not, I don’t know.

If you prefer tap to tape, then either one needs to find how to integrate airtap with tap, or choose a different tool to run browser testing, see #14 and #21.

@vweevers
Copy link

In theory Airtap integrates with any test harness that produces TAP output using console.log(). Maybe tap produces slightly different TAP output than tape breaking certain airtap expectations. If so I would consider that a bug in airtap and I would be willing to fix that. Or maybe it's a browserify issue. Do you have a stack trace?

@denis-sokolov
Copy link
Contributor Author

I have tried using airtap as their starting guide suggests, where the airtap is the top-level executable that runs the test file itself. I am not sure I understand how the alternative workflow would work. TAP is test output format, is it not? But we want not just the output, we want to run the tests themselves in airtap.

The error itself is probably not useful. But perhaps I don’t know how to run airtap with tap.

 Can't walk dependency graph: Cannot find module 'bluebird' from './secure-json-parse/node_modules/trivial-deferred/index.js'
    required by ./secure-json-parse/node_modules/trivial-deferred/index.js
    at ./secure-json-parse/node_modules/resolve/lib/async.js:116:35
    at processDirs (./secure-json-parse/node_modules/resolve/lib/async.js:269:39)
    at isdir (./secure-json-parse/node_modules/resolve/lib/async.js:276:32)
    at ./secure-json-parse/node_modules/resolve/lib/async.js:25:69
    at FSReqCallback.oncomplete (fs.js:183:21)

@Eomm
Copy link
Member

Eomm commented Jan 18, 2021

I would include this change in PR #21, since it is related to it, and if tape is necessary to reach that goal without too much burden it would be ok in my opinion

@denis-sokolov
Copy link
Contributor Author

denis-sokolov commented Jan 18, 2021

I apologize for being a little confused. Could make it very clear to me when you’re having a discussion and providing a personal opinion vs when you have made a decision as a community of maintainers that we can follow through with? Thanks.

@vweevers
Copy link

I am not sure I understand how the alternative workflow would work. TAP is test output format, is it not? But we want not just the output, we want to run the tests themselves in airtap.

@denis-sokolov In a nutshell (at risk of derailing this thread): Airtap monkeypatches console.log() in order to intercept TAP output and then loads the user's test script, which is expected to produce TAP output using tape, tap or other.

The error itself is probably not useful.

It's very useful, because Can't walk dependency graph comes from browserify. This tells us that tap is not compatible with browserify and perhaps with browsers in general. Such issues can be fixed but tape is already known to be compatible.

@kibertoad
Copy link
Member

kibertoad commented Jan 18, 2021

@denis-sokolov This is a good practice in general - keeping cohesive changes in a single PR, so that in the future it is easier to see which changes happened for what reason, in context. Sorry if the way it was phrased sounded like a personal attack; I think the sentiment behind the message is generally shared by the maintainer team. Each PR should provide an atomary, incremental improvement, and in this case it really feels like this is not a standalone improvement but merely a part of #15 and #21, since it only brings value when combined with all of them.

@kibertoad
Copy link
Member

kibertoad commented Jan 18, 2021

when you’re having a discussion and providing a personal opinion vs when you have made a decision as a community of maintainers that we can follow through with.

Just to provide a bit of clarification with regards to this particular request. It is not uncommon to phrase direct requests as indirect observations (it might be a good idea to...) in order to reduce communication friction by making communication less violent. Generally speaking, more imperative communication is perceived as less polite, since it indicates superiority of one of the communication participants. However, such subtleties might be lost in translation between different communication cultures, which, I believe, might have caused a confusion here.

If you are curious about this subject, I would recommend reading up on nonviolent communication and indirect communication, e. g. https://socialcommunication.truman.edu/attitudes-emotions/polite-indirect/

@denis-sokolov denis-sokolov changed the title Swith testing to tape Switch testing to tape Jan 18, 2021
@denis-sokolov
Copy link
Contributor Author

Closing in favor of continuing the conversation in #21.

@vweevers, I appreciate the insight in how Airtap works internally. Explains why you had an expectation that it could work, which was previously deeply perplexing to me.

@kibertoad, I appreciate the clarity on the preference for this to be merged in to #21. For what it’s worth, I think your opinion that this case is clear cut best practice may be up for reconsideration. There’s a sliding scale of different trade-offs choosing how to atomize changesets and you could see others having different judgement calls about these uncountable preferences.

@kibertoad, I also appreciate the explanation behind the communication style. Needless to say, the global cultural differences we come from mean we need to be cautious and clear.

@denis-sokolov denis-sokolov deleted the tape branch January 18, 2021 18:49
@kibertoad
Copy link
Member

@denis-sokolov I agree, it is, indeed, subjective.

@Eomm
Copy link
Member

Eomm commented Jan 18, 2021

I totally agree with @kibertoad

I apologize for being a little confused. Could make it very clear to me when you’re having a discussion and providing a personal opinion vs when you have made a decision as a community of maintainers that we can follow through with? Thanks.

I would just add to the discussion here, that all the decisions are shared and discussed publicly following the consunsu seeking process.
I'm sorry if this has been odd

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.

6 participants