feat: webworker flags #99

daviddias · 2017-02-09T15:37:38Z

This seems to get the job done, however, due to lack of tests and all of this flags everywhere, I'm not 100% if I didn't miss anything.

dryajov · 2017-02-09T17:08:06Z

@diasdavid I think this changes need to happen in bin/test, the dom/webworker flags are being checked there, if anything they probably need to be removed from there. I think either way works.

dignifiedquire

As @dryajov said, there need to be also changes in the bin/ files.

Yes there are no tests, and flags are currently horrible hacky..

dignifiedquire · 2017-02-09T17:43:11Z

tasks/test/browser.js

@@ -27,7 +27,8 @@ module.exports = (gulp) => {
  })

  gulp.task('test:karma:webworker', (done) => {
-    if (util.env.dom) {
+    // Not execture tests in the webworker


executure is not a word I know ;)

daviddias · 2017-02-09T19:47:03Z

@dryajov @dignifiedquire applied your CR

daviddias · 2017-02-09T20:14:59Z

Apparently, failing at WebWorker tests doesn't make the tests fail.

dryajov · 2017-02-09T20:22:49Z

@diasdavid Thats bad... going to take a look at it.

dignifiedquire · 2017-02-09T20:22:54Z

Apparently, failing at WebWorker tests doesn't make the tests fail

that's a bug Oo

dryajov · 2017-02-10T00:15:54Z

Apparently, failing at WebWorker tests doesn't make the tests fail

being tracked in #100

dryajov · 2017-02-10T17:32:19Z

tasks/test/browser.js

-      'test:karma:webworker',
-      utils.exitOnFail(done)
-    )
+    if (util.env.webworker === 'only') {


I don't think we need to check for the flags here and in bin/test... I think its better if we do it in one place. IMHO, bin/test is probably better and the leaving tasks/test/browser the way it was before but with the new flag names.

Sounds good, would you like to push those changes to this branch?

Sure, I'll make them.

@diasdavid I've made the changes but I can't push to this repo... I can create a new PR or you can give me perms, whatever works.

@diasdavid here is the new PR in case we want to do that - #103

Thank you :)

dryajov · 2017-02-14T02:29:42Z

config/karma.conf.js

@@ -94,6 +94,6 @@ module.exports = function (config) {
    singleRun: false,
    concurrency: concurrency,
    browserNoActivityTimeout: timeout,
-    failOnEmptyTestSuite: false
+    failOnEmptyTestSuite: true


@diasdavid BTW, this should stop broken tests from slipping through.

I've pulled this change into master in its own commit

daviddias · 2017-03-08T17:09:10Z

@dryajov @dignifiedquire what is needed to finish this PR?

dignifiedquire · 2017-03-08T17:11:14Z

README.md

-If the needed environment variables are set, tests are also run on [Sauce Labs].
-You will need
+- `--webworker=false`
+- `--webworker=only`


I'm not convinced these are an actual improvement over what we currently have.

Could you elaborate on why?

For me the current --dom and --webworker flags work well enough. Changing them to the above does not make it more intuitive/clear what they do for me. So it comes down to subjective preference and at that point I would rather stick with what we have than change it.

I agree with @dignifiedquire I think those flags make sense in general, as opposed to the --webworker=true/only combination.

There is a node and browser env flag in aegir and historically browser only meant that it would run the 'main thread'.

I strongly feel that --dom translated that it will only run in the main thread.
I'm also not convinced that we should run in webworkers by default
I also feel that having two different flags to disable/enable webworker tests is not good UI.

I am not saying the current version is particularly good, in reality the whole thing just needs refactoring and clean up, this was never intended to support many options. But this PR for me doesn't actually fix the issues which is why I am hesitant to merge it.

IMO, both options are equally confusing and the whole thing needs refactoring into something a little more cohesive, as @dignifiedquire already said. I like my original options of --dom/--webworker, but I'm obviously biased :)

Who else cares about this? Perhaps we should put it up for a vote? 🍭

@diasdavid

I'm also not convinced that we should run in webworkers by default

The reason I went with this is because a) most of our code needs to run in WW either way[1] and b) it was a lot less work to enable them by default rather than selectively enable/disable them.

Any specific reason as to why this is not the right path?

1 - We have a lot less code that would not run in a WW than otherwise, the ones that come to mind that should not be enabled in WW/Browsers are the transports and storage engines that are not natively supported by the browser, the rest should work across the board. Browser tests are already disabled for those either way, so the only one left that should not be enabled in WW is WebRTC.

We can keep the --dom and --webworker, that is the least of the concerns at the moment :)

When I requested for feedback on it, it was positive and that took me to bring up a PR. What this reminds me is that aegir needs desperately a Roadmap with the clear direction of what is missing and what will make it 'done'. Right now it is a core piece of every JS project withing 4 organizations (ipfs, libp2p, multiformats and IPLD) and it is has become more glue code than a tool that is easy to play with.

daviddias added the status/in-progress In progress label Feb 9, 2017

daviddias requested a review from dignifiedquire February 9, 2017 15:37

daviddias mentioned this pull request Feb 9, 2017

Awesome WebWorkers Support ipfs/js-ipfs#725

Closed

7 tasks

dignifiedquire suggested changes Feb 9, 2017

View reviewed changes

feat: webworker flags

e4c52c9

daviddias force-pushed the feat/webworker-flags branch from 588a48c to e4c52c9 Compare February 9, 2017 19:46

daviddias added a commit to libp2p/js-libp2p-spdy that referenced this pull request Feb 9, 2017

quick workaround until ipfs/aegir#99

fa18da7

This was referenced Feb 9, 2017

libp2p-spdy@0.10.4 breaks build ⚠️ libp2p/js-libp2p-kad-routing#94

Closed

libp2p-spdy@0.10.4 breaks build ⚠️ libp2p/js-libp2p-kad-record-store#73

Closed

dryajov mentioned this pull request Feb 10, 2017

failing tests are incorrectly passing under webworker runs #100

Closed

dryajov reviewed Feb 10, 2017

View reviewed changes

fix: consolidate --webworker=true/false flags into once place (#103)

e4afe1f

dryajov reviewed Feb 14, 2017

View reviewed changes

dignifiedquire reviewed Mar 8, 2017

View reviewed changes

daviddias added status/ready Ready to be worked and removed status/in-progress In progress labels Mar 9, 2017

daviddias closed this Mar 11, 2017

daviddias removed the status/ready Ready to be worked label Mar 11, 2017

daviddias mentioned this pull request Mar 11, 2017

aegir.next() #113

Closed

daviddias deleted the feat/webworker-flags branch March 11, 2017 20:13

This was referenced Mar 21, 2017

libp2p-spdy@0.10.6 breaks build ⚠️ libp2p/js-libp2p-kad-routing#118

Closed

libp2p-spdy@0.10.6 breaks build ⚠️ libp2p/js-libp2p-kad-record-store#95

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: webworker flags #99

feat: webworker flags #99

daviddias commented Feb 9, 2017

dryajov commented Feb 9, 2017

dignifiedquire left a comment

dignifiedquire Feb 9, 2017

daviddias commented Feb 9, 2017

daviddias commented Feb 9, 2017 •

edited

Loading

dryajov commented Feb 9, 2017

dignifiedquire commented Feb 9, 2017

dryajov commented Feb 10, 2017

dryajov Feb 10, 2017

daviddias Feb 12, 2017

dryajov Feb 12, 2017

dryajov Feb 14, 2017

dryajov Feb 14, 2017 •

edited

Loading

daviddias Feb 14, 2017

dryajov Feb 14, 2017

dignifiedquire Mar 8, 2017

daviddias Mar 9, 2017

daviddias commented Mar 8, 2017

dignifiedquire Mar 8, 2017

daviddias Mar 8, 2017

dignifiedquire Mar 8, 2017 •

edited

Loading

dryajov Mar 9, 2017 •

edited

Loading

daviddias Mar 9, 2017

dignifiedquire Mar 9, 2017

dryajov Mar 10, 2017 •

edited

Loading

dryajov Mar 10, 2017 •

edited

Loading

daviddias Mar 11, 2017

feat: webworker flags #99

feat: webworker flags #99

Conversation

daviddias commented Feb 9, 2017

dryajov commented Feb 9, 2017

dignifiedquire left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias commented Feb 9, 2017

daviddias commented Feb 9, 2017 • edited Loading

dryajov commented Feb 9, 2017

dignifiedquire commented Feb 9, 2017

dryajov commented Feb 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dryajov Feb 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias commented Mar 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dignifiedquire Mar 8, 2017 • edited Loading

Choose a reason for hiding this comment

dryajov Mar 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dryajov Mar 10, 2017 • edited Loading

Choose a reason for hiding this comment

dryajov Mar 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias commented Feb 9, 2017 •

edited

Loading

dryajov Feb 14, 2017 •

edited

Loading

dignifiedquire Mar 8, 2017 •

edited

Loading

dryajov Mar 9, 2017 •

edited

Loading

dryajov Mar 10, 2017 •

edited

Loading

dryajov Mar 10, 2017 •

edited

Loading