-
-
Notifications
You must be signed in to change notification settings - Fork 17
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
Add testing in browsers #21
Conversation
1b0f2a3
to
04b25d9
Compare
@vweevers do you think we could disable the polifills somehow? |
IMHO, there are too many tangential PRs that are created specifically to support this one PR. I would rather see a single PR that implements the thing you want to implement instead of making decisions on many different ones that are out of context. |
@mcollina Do you want to disable all polyfills (I'm not sure tape will like that) or only browserify:
- ignore: buffer |
Changing the test tool is seemingly only necessary if this PR is to be accepted. Therefore, it doesn't bring value without this PR being accepted. Same for #15. |
@jsumners, I apologize, I do not see value in discussing the merits of individuals pull requests and our individual preferences for workflows. If you find this discussion crucial, you are welcome to close all my pull requests and tell me to get lost. If you are not interested in that, let’s focus on the proposed changes and their integration. |
The summary of the tap vs tape conversation:
@RafaelGSS, @Eomm, adding you here after the conversation in #16 was moved here. |
04b25d9
to
5d3b7da
Compare
5d3b7da
to
fef9395
Compare
This is primarily to support a future addition of airtap which relies on tape.
8d611fa
to
3179e6a
Compare
@kibertoad, @Eomm, the PR is now ready for another round of review at your convenience. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
@denis-sokolov Btw, why is this not failing on browsers despite not including #15? |
What's the benefit of this change if it doesn't reproduce actual behaviour of browsers, though? |
It’s unlucky it does not catch the Buffer-related issue in the browser, but it may still catch some other behavior differences some time. If you decide the maintenance burden of airtap is not worth that minor benefit, it makes perfect sense to reject this PR, I won’t take it personally. :-) It’s likely there is testing tooling that allows us to do better testing in the browser. Unfortunately, we’ve uncovered the Buffer issue in airtap only after implementing this PR. Right now there is not enough incentive to start over with a new tool. ¯\_(ツ)_/¯ |
@denis-sokolov Fair point, also @vweevers promised to look into configuring airtap, so hopefully this can actually be addressed in an optimal way. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
As per conversation in #15, added airtap for test running in the browsers.
The diff includes changes from PRs #16, #18, #20, please wait for those to be merged for the diff to be nicer.
Ironically, these tests do not catch the Buffer issue that #15 fixes. During the bundling for testing, airtap uses Browserify, which packages a Buffer polyfill in. I could not find a way to disable that and keep the rest of the tooling working. But unrelated to #15, perhaps this tooling is beneficial for the project.
Checklist
npm run test
andnpm run benchmark
and the Code of conduct