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

Test DOMPurify with a Browser grid #61

Closed
fxa opened this issue May 19, 2015 · 35 comments
Closed

Test DOMPurify with a Browser grid #61

fxa opened this issue May 19, 2015 · 35 comments

Comments

@fxa
Copy link

fxa commented May 19, 2015

DOMPurify is designed to run in browsers. In ALL browsers. I would like to make some minor changes, but would like to be sure not to break anything. So I would like to have tests in a LOT of browsers in different versions and OSs. Wouldn't it be nice to have tests with a lot of browser/os combinations? There are some vendors, who offer that for free for open source projects like BrowserStack, SauceLabs or BrowserSwarm (I did not evaluate any of them for features or even fitness).
I think, this test support is more important, than my suggesting changes.
Are there any plans to do things like that? If not, I would like to do some research in this area.

@cure53
Copy link
Owner

cure53 commented May 19, 2015

That would make sense indeed, research in this area is highly appreciated!

Thanks :)

@fhemberger
Copy link
Contributor

I know some people at SauceLabs, maybe they can help us with that …

EDIT: That really sounds incredible (https://saucelabs.com/opensauce/)

If you have an open source project, you just need to enter your project’s repository URL and a description of what it is when you create your account. You’ll automatically be enrolled in the plan, which provides unlimited testing minutes on up to three parallel VMs and access to all our features, including 450+ OS/browser combos, screenshots, debugging tools and more.

@Joris-van-der-Wel
Copy link
Contributor

Would also be great if the the tests could be refactored so that they can run without a html file, do not require jquery and do not require XHR. The html file would only serve as a runner, but it does not contain any of the tests itself. It would make this script much simpler: https://joris-van-der-wel.github.io/DOMPurify-test-jsdom.js

@Joris-van-der-Wel
Copy link
Contributor

I spent some time figuring this out for a different project (using Sauce Labs Open Sauce).

I first tried the runner "testem", which is great if you are only testing on your own machine. However the integration with things such as saucelabs is not yet mature. So I switched to the runner karma instead.

Karma understands QUnit, however I used it with mocha + browserify myself. You will have to move over the tests to their own javascript file though (instead of that big .html file). It is probably easiest to use browserify for this, so that you will no longer need a XHR request to load that json file: require('./expect.json')

I made two karma configurations, the first one tests with jsdom and then simply waits for you to manually visit a special url. The second one launches a large amount of browsers on saucelabs (with a build id, which gives you a nice status image).

I configured a large amount of browsers in my project which is kind a cumbersome to do unless you add a few convenience functions

If you launch karma with all those browsers, it will try to test them all at once... So I made a simple grunt script to execute them one by one.

The downside of sauce labs is that sometimes a browser will simply fail to launch (even with the built in retrying of karma). This is why some browsers are marked as gray on my package page: https://www.npmjs.com/package/name-overrides-builtin. For some strange reason, iOS browsers almost always fail to launch. I only got those to work a handful number of times.

Yes, this post is also plugging my library ;)

@cure53
Copy link
Owner

cure53 commented Aug 2, 2015

@fxa @Joris-van-der-Wel @fhemberger Hi all! I wasn't following this ticket, what's the status here?

@fhemberger
Copy link
Contributor

Sorry, I've been completely out of touch the last couple of weeks. I don't know about any changes.

@fxa
Copy link
Author

fxa commented Aug 2, 2015

I don't have the time to do anything in this direction. i just wanted to submit a suggestion to improve the quality of DOMPurify

@cure53
Copy link
Owner

cure53 commented Aug 2, 2015

Are there any plans to do things like that? If not, I would like to do some research in this area.

Okay then :)

@cure53
Copy link
Owner

cure53 commented Aug 25, 2015

Since we don't have any progress here, I would suggest to close this issue. Any objections?

@fhemberger
Copy link
Contributor

Why not leave it open as feature todo? Just flag it with a tag "future", "enhancement" or "discussion".

@tdeekens
Copy link
Collaborator

I have experience in karma testing via BrowserStack's Selenium grid. They usually give away free slots for Open Source. Any objections against TravisCI and BrowserStack? Otherwise I'd just work on it in a fork and give you folks the chance to merge it back in.

@fhemberger
Copy link
Contributor

I'm fine with that, we can use either BrowserStack or SourceLabs. Having browser testing would be a huge improvement.

@cure53
Copy link
Owner

cure53 commented Sep 15, 2015

@tdeekens Perfectly fine with that, thanks!

@cure53
Copy link
Owner

cure53 commented Sep 16, 2015

@tdeekens Alright, we have BrowserStack access, I connected Travis-CI. Looks like we're set - aside from the credentials to be added. Let's set up the rest tomorrow?

@tdeekens
Copy link
Collaborator

Awesome. Sure thing.

@fhemberger
Copy link
Contributor

I have some remarks about the new testing process:

  • Travis shows the following error:
> dompurify@0.6.6 jshint /home/travis/build/cure53/DOMPurify
> node node_modules/jshint/bin/jshint purify.js || true
ERROR: Can't open purify.js
Hash: 8344a6c0a9b3c44a5636
Version: webpack 1.12.2
Time: 15ms
webpack: bundle is now VALID.
webpack: bundle is now INVALID.
Hash: 04226e862d2bca35b9e3
Version: webpack 1.12.2
Time: 308ms
  • You should add sudo: false to .travis.yml to use the faster, containerized infrastructure
  • Can we add testing for node.js@4.0.0 as well (w/o Browserstack)?
  • Why are we running bower install?

@tdeekens
Copy link
Collaborator

#93 fixes the jshint issue while also adding sudo: false to travis.yml.

We're running bower install to pull down jQuery which is injected into the browser via karma here.

I guess we could run tests against node w/o BrowserStack as QUnit should support it, right?

@tdeekens
Copy link
Collaborator

...just saw that you'd at least need some sort of DOM within the node environment. I guess something like jsdom could do. I don't see much benefit if one runs it against e.g. PhantomJS as a CI run currently only needs 2 minutes with 5 browsers hooked to it.

@Joris-van-der-Wel
Copy link
Contributor

It would be useful if dompurify was tested against jsdom in addition to other browsers. You may want to skim #29 and #64

@fhemberger
Copy link
Contributor

@tdeekens Yes, we used jsdom so far, it's the only working DOM implementation in Node.js I know of. It would be super helpful if we could run the tests against it as well, so we can make sure the purifying also works on the server side.

@cure53
Copy link
Owner

cure53 commented Sep 17, 2015

@tdeekens There is one more concern - the local tests don't work yet. I assume it has to do with the environment vars in the karma configuration file:

              Asset    Size  Chunks             Chunk Names
test/purify.spec.js  306 kB       0  [emitted]  test/purify.spec.js
chunk    {0} test/purify.spec.js (test/purify.spec.js) 113 kB [rendered]
    [0] ./test/purify.spec.js 15.8 kB {0} [built]
    [1] ./src/purify.js 25.2 kB {0} [built]
    [2] ./test/fixtures/expect.js 72.1 kB {0} [built]
webpack: bundle is now VALID.
17 09 2015 20:49:36.383:INFO [karma]: Karma v0.13.9 server started at http://localhost:9876/
17 09 2015 20:49:36.393:WARN [launcher]: Can not load "bs_win81_ie_11"!
  Error: Username is required.
    at Version.Client (/git/DOMPurify/node_modules/karma-browserstack-launcher/node_modules/browserstack/lib/browserstack.js:24:9)
    at new Version (/git/DOMPurify/node_modules/karma-browserstack-launcher/node_modules/browserstack/lib/browserstack.js:248:10)
    at Object.module.exports.createClient (/git/DOMPurify/node_modules/karma-browserstack-launcher/node_modules/browserstack/lib/browserstack.js:365:10)

Any idea what needs to be set / changed to make it work again?

@cure53
Copy link
Owner

cure53 commented Sep 17, 2015

I am thinking, this might be the problem?

https://github.com/cure53/DOMPurify/blob/master/test/karma.conf.js#L22

@tdeekens
Copy link
Collaborator

Yeah. Just export the BS_USERNAME and BS_ACCESSKEY in your profile.

@cure53
Copy link
Owner

cure53 commented Sep 17, 2015

Why can it not grab the data from here?

https://github.com/cure53/DOMPurify/blob/master/.travis.yml#L10

@tdeekens
Copy link
Collaborator

Even so I am not super fond of it but it seems to be the general way to avoid checking in those credentials. You can add team members in your BrowserStack account here.

Still you'd need to export your raw username and accesskey not the encrypted one for TravisCI.

@tdeekens
Copy link
Collaborator

I wonder why we have six entries in the travis.yml not two: one username one accesskey. Maybe clean up later once you got it all running locally.

@cure53
Copy link
Owner

cure53 commented Sep 17, 2015

I'd suggest to meet up tomorrow and have a look?

@tdeekens
Copy link
Collaborator

Sure.

@cure53
Copy link
Owner

cure53 commented Sep 17, 2015

So be it :)

@cure53
Copy link
Owner

cure53 commented Sep 18, 2015

Sweet, we got the automation plan from BrowserStack and now we have all tests running in eight different browsers:

      'bs_win81_ie_11',
      'bs_win8_ie_10',
      'bs_mavericks_chrome_44',
      'bs_yosemite_firefox_40',
      'bs_yosemite_safari_8',
      'bs_win81_opera_31',
      'bs_win7_firefox_12',
      'bs_win81_chrome_22'
    ],

Anything that comes to mind we should add as well?

@tdeekens
Copy link
Collaborator

Good question. I'd say test for the worst (some really old crappy version) and latest (as most of them are evergreen self updating browsers anyway) on Mac and Win. Still, I am no expert on or buggy intermediate versions of browsers are.

@cure53
Copy link
Owner

cure53 commented Sep 18, 2015

I am also planning to add Android and iOS just in case. We should be able to run on older FF and Chrome as well but I have to test manually first...

@tdeekens
Copy link
Collaborator

We just have to make sure not to run on Android and iOS emulators for those tests. I had some mobile tests activated for grunt-dactylographsy which turned out to have very long queue times cause devices will be booted etc.

Maybe having desktop test run on all commits and mobile as an addition on release commits if that is possible and makes sense.

@tdeekens
Copy link
Collaborator

...do we wanna close this one? I suggest opening another issue covering running tests against node/iosjs with jsdom.

@cure53
Copy link
Owner

cure53 commented Sep 19, 2015

Yes, I agree. Similarly for mobile.

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

No branches or pull requests

5 participants