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.

Sign up for GitHub

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

feat: webworker flags #99

Closed
wants to merge 2 commits into from
Closed

feat: webworker flags #99

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e4c52c9
feat: webworker flags
daviddias Feb 9, 2017
e4afe1f
fix: consolidate --webworker=true/false flags into once place (#103)
dryajov Feb 14, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,10 +77,12 @@ $ aegir-test browser
$ gulp test:browser
```

If you want to run tests in a webworker you can also pass `--webworker` as a flag to enable that.
Browser tests by default run inside the main thread and a webworker thread. If you want to disable or only run your tests in a webworker, you can do it by passing the flag:

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Could you elaborate on why?

Copy link
Member

@dignifiedquire dignifiedquire Mar 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

@dryajov dryajov Mar 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@dryajov dryajov Mar 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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? 🍭

Copy link
Member

@dryajov dryajov Mar 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.


If the needed environment variables are set, tests are also run on [Sauce Labs]. You will need:

- `$SAUCE=true`
- `SAUCE_USERNAME=<username>`
Expand Down
4 changes: 2 additions & 2 deletions bin/test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@ require('../src/gulp-log')(gulp)
require('../gulp')(gulp, ['test'])

if (args.browser) {
if (args.dom) {
if (args.webworker === false) {
gulp.start('test:karma')
} else if (args.webworker) {
} else if (args.webworker === 'only') {
gulp.start('test:karma:webworker')
} else {
// run both dom and webworker tests by default
Expand Down
2 changes: 1 addition & 1 deletion config/karma.conf.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,6 @@ module.exports = function (config) {
singleRun: false,
concurrency: concurrency,
browserNoActivityTimeout: timeout,
failOnEmptyTestSuite: false
failOnEmptyTestSuite: true
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

awesome

})
}
3 changes: 2 additions & 1 deletion tasks/test/browser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ module.exports = (gulp) => {
})

gulp.task('test:karma:webworker', (done) => {
if (util.env.dom) {
// Skip running tests in a webworker
if (util.env.webworker === 'false') {
return done()
}

Expand Down