Fix failing tests across browsers #9

cvle · 2016-11-20T09:34:19Z

cvle · 2016-11-20T09:48:54Z

I moved the test file into a separate folder, and I'm trying to adapt the scripts in package.json, but I don't really get the purpose of npm run build:tests ?

cvle · 2016-11-20T09:49:54Z

Everything else is ready.

cvle · 2016-11-20T11:04:12Z

@kof I did some changes in the last hour, in case you were already checking out this PR.

kof · 2016-11-20T10:53:18Z

test/utils.js

@@ -0,0 +1,15 @@
+import * as bowser from 'bowser'


Started to use https://github.com/cssinjs/jss-vendor-prefixer/blob/master/package.json#L52 for this, is bowser better, should we have one consistent dep for it?

Looking at both projects, bowser looks more mature, well tested, and actively maintained. I would change to bowser.

kof · 2016-11-20T10:57:53Z

tests.webpack.js

@@ -1,2 +1,2 @@
-const context = require.context('./src', true, /\.test\.js$/)
+const context = require.context('./test', true, /\.test\.js$/)


no, it was fine before, "test" dir dosn't exist how can this work?
The "tests" dir is generated in order to run tests from the main repo, as we probably not going to do this for this repo, we can remove build:tests:lib and build:tests and just rename build:tests:local to build:tests

I moved src/index.test.js to test/integration.test.js and added a helper file test/utils.js.

What is the purpose of build:tests:local?

Oh, lets keep index.test in /src and add /test/utils.js

build:tests:local is to run it via tests.html in the browser, sometimes you need that for better debugging.

Why do you want to keep the file in src/index.test.js? For me this indicates that it is an unit test only for the index file, but it is an integration test for the whole project. For me that's a bit confusing. What if you were to add unit tests in the future?

kof · 2016-11-20T10:59:49Z

test/integration.test.js

-      if (prop !== 'animation') {
-        expect(prop).to.be(`${prefix.css}animation`)
-      }
+    browser("requires prefix for transform property", {


I still think this way is sort of a hack. A real good solution would be to compare results in every browser with already known predefined results from caniuse.

Why is this a hack? I used the values from caniuse as they are static and won't ever change, there is no need to query the api and introduce another dependency that could break.

You take specific browsers, where you know the outcome and you do an assertion with expected output. You selectively run an assertion. Not really a hack, but could be better: we run always the assertion and compare the results with the result expected in a certain browser. Without maintaining the versions etc. Just get the result for the current browser from caniuse (not http api, there is a static version) and then compare it with what we got from the browser.

I see what you mean. I'll checkout how complicated it is to use the api directly.

cvle · 2016-11-20T16:09:37Z

I'm finally done. This one was really tricky.

Currently mocha report suffers from this issue: litixsoft/karma-mocha-reporter#84.

kof · 2016-11-20T16:11:11Z

I think I saw that bug as well, but didn't dig to understand whats going on there.

cvle · 2016-11-20T16:13:21Z

I just fixed some remaining linter errors.

kof · 2016-11-20T16:15:03Z

test/utils.js

+ * @param {String} feature - see full list https://github.com/Fyrd/caniuse/tree/master/features-json
+ * @returns {{level: 'unknown' | 'none' | 'partial' | 'full', needPrefix: boolean}}
+ */
+export function getSupport(feature) {


wow, that sounds complex, I wonder why nobody did this logic before ...

This library you mentioned earlier doesn't work in the browser: https://github.com/Nyalab/caniuse-api and there was no other library around. Had to do it myself ^^

kof · 2016-11-20T16:17:28Z

test/utils.js

+ * @param {String} feature - see full list https://github.com/Fyrd/caniuse/tree/master/features-json
+ * @returns {{level: 'unknown' | 'none' | 'partial' | 'full', needPrefix: boolean}}
+ */
+export function getSupport(feature) {


wondering how we can reuse that logic across projects, for e.g. I know for sure it is needed in jss-vendor-prefixer as well.

what if we release a separate package, which will wrap caniuse db and provide the api we need for this sort of testing?

That's a good idea. I'll setup a separate package tomorrow.

Cool, lets wait with the merge then.

kof · 2016-11-21T08:57:39Z

Well we can refactor file names any time, its easy. For now I started to use that fileName.test.js convention in all plugins where we don't separate integration and unit tests. Also if they are small integration and unit tests can be all in the same file, normally people separate them in large projects when speed of running tests becomes a problem.

On Nov 21, 2016, at 3:41 AM, Chi Vinh Le [email protected] wrote:

@cvle commented on this pull request.

In tests.webpack.js #9:

@@ -1,2 +1,2 @@
-const context = require.context('./src', true, /.test.js$/)
+const context = require.context('./test', true, /.test.js$/)
Why do you want to keep the file in src/index.test.js? For me this indicates that it is an unit test only for the index file, but it is an integration test for the whole project. For me that's a bit confusing. What if you were to add unit tests in the future?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub #9, or mute the thread https://github.com/notifications/unsubscribe-auth/AADOWP3X8Kf8saudGFXK-LNzLG69CVVpks5rAQTkgaJpZM4K3foh.

cvle · 2016-11-21T10:55:55Z

I see, I was just curious for the reason.

cvle · 2016-11-21T14:10:27Z

Almost done. Waiting for some free slots on your browserstack account to fix remaining issues if any.

cvle · 2016-11-21T14:36:44Z

Everything passed! 👍
This is good to go.

Check out the new package at https://github.com/wikiwi/caniuse-support.

kof · 2016-11-23T11:02:34Z

Awesome

kof · 2016-11-23T11:04:38Z

karma.conf.js

    })

    config.browserStack = {
      username: browserStackUserName,
      accessKey: browserStackAccessKey,
-      captureTimeout: 10000
+      captureTimeout: 3e5


this is 300000ms, tests may stuck for a long time with such a timeout, the values I had there was what I tried is the best considered browserstack.

See this issue: karma-runner/karma-browserstack-launcher#61.
I had issues with testing on the mobile android device which got restarted on every run and then got stuck forever. I had to kill the worker on each run manually in order to free the slot again. I took the values from the issue above, which are supposed to be timeout values given by the browserstack support.

kof · 2016-11-23T11:06:02Z

src/index.test.js


 import {prefix, supportedProperty, supportedValue} from './index'

+console.log(`Detected browser: ${currentBrowser.id} ${currentBrowser.version}`)


maybe disbable eslint just for this line, because its very likely that we forget console.log statements after debugging and now lint won't warn.

I'll do that.

kof · 2016-11-23T11:06:40Z

src/index.test.js

-      if (prop !== 'animation') {
-        expect(prop).to.be(`${prefix.css}animation`)
-      }
+    it('should prefix if needed', function () {


any reason why you prefer to have "function" here?

oh I see, you need this.skip

Otherwise I cannot call this.skip().

kof · 2016-11-23T11:16:51Z

Hmmm ok then.

On Nov 23, 2016, at 12:14 PM, Chi Vinh Le [email protected] wrote:

@cvle commented on this pull request.

In karma.conf.js #9:
 })
 config.browserStack = {
   username: browserStackUserName,
   accessKey: browserStackAccessKey,
 captureTimeout: 10000
 captureTimeout: 3e5
See this issue: Disconnection issues with Mobile Devices karma-runner/karma-browserstack-launcher#61 Disconnection issues with Mobile Devices karma-runner/karma-browserstack-launcher#61.
I had issues with testing on the mobile android device which got restarted on every run and then get stuck forever. I had to kill the worker on each run manually in order to free the slot again. I took the values from the issue above, which are supposed to be timeout values given by the browserstack support.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub #9, or mute the thread https://github.com/notifications/unsubscribe-auth/AADOWMUPVqZlnGd3uFAtutdNkGzivP-nks5rBCAGgaJpZM4K3foh.

kof · 2016-11-23T11:17:33Z

Looks good. We can merge it. One more idea would be to generate tests for every known property and do those checks. This way we know if anything else goes wrong.

cvle · 2016-11-23T11:48:24Z

What we can do is to map property names to the appropriate caniuse feature name, and use this to run our prefix test. Here is a list we could rely on for which properties to test: http://shouldiprefix.com/

Let's leave this up to a following PR, and merge this first. Suggestions have been applied.

kof · 2016-11-23T11:57:26Z

merged

cvle · 2016-11-23T12:09:42Z

Just looking at the travis build, I noticed that currently npm install triggers prepublish which runs npm run all, so the travis test runs browser stack twice and everyone tinkering with this package also runs karma test during install.

kof · 2016-11-23T12:15:57Z

travis test runs browser stack twice

u sure? It should only do install which runs the tests

so the travis test runs browser stack twice and everyone testing this package also runs karma test during install.

yes, but only if you are installing it in a cloned repository, not if it is a dependency, so it is only for css-vendor developers the case

kof · 2016-11-23T12:17:07Z

I think npm recently fixed that by adding a separate hook for what we need, the intend was always to test before publishing, not all the other cases. Also there is a separate package which solves this problem. Need to do that for all repos though.

cvle · 2016-11-23T12:18:22Z

Travis just ran test twice during npm install and npm test checkout the logs.

kof · 2016-11-23T12:25:25Z

hmm, where does install and test come from, it should be just install

cvle · 2016-11-23T12:25:30Z

Alternatively you can think about publishing using travis.
My approach currently looks like this:

Run ~~npm release patch~~ npm version patch
Run git push --follow-tags
Let travis deploy the package.

cvle · 2016-11-23T12:26:06Z

hmm, where does install and test come from, it should be just install

That's the default travis behavior.

kof · 2016-11-23T12:27:21Z

Shit))) we need to fix that, running tests twice on jss repo as well then. And yes, seems to be a good idea to publish the package over travis.

kof · 2016-11-23T12:28:13Z

Oh actually we may be able to say what to run in travis.yaml.

cvle · 2016-11-23T12:29:00Z

Yes, if you define the script part, travis will not apply its default behavior.

kof · 2016-11-23T12:30:21Z

yeah, so we need either this or publish over travis. I tend to publish over travis and remove the prepublish thing then.

cvle · 2016-11-23T12:31:22Z

I recommend publishing over travis too 👍

cvle · 2016-11-23T12:42:03Z

My mistake before: I wrote npm release patch but it is npm version patch. Checkout my project https://github.com/wikiwi/caniuse-support to get some ideas.

E.g. in this project I just run npm run release patch && git push --follow-tags for a patch release.

cvle changed the title ~~Fix failing tests~~ Fix failing tests across browsers Nov 20, 2016

cvle changed the title ~~Fix failing tests across browsers~~ [WIP] Fix failing tests across browsers Nov 20, 2016

cvle force-pushed the fix-failing-tests branch 2 times, most recently from 1bc4667 to dceaeda Compare November 20, 2016 10:33

kof reviewed Nov 20, 2016

View reviewed changes

cvle force-pushed the fix-failing-tests branch from dceaeda to 00e75b2 Compare November 20, 2016 15:47

cvle changed the title ~~[WIP] Fix failing tests across browsers~~ Fix failing tests across browsers Nov 20, 2016

cvle force-pushed the fix-failing-tests branch 3 times, most recently from cc3eacb to ad88c0d Compare November 20, 2016 16:06

cvle force-pushed the fix-failing-tests branch 2 times, most recently from 06b1284 to 11d6055 Compare November 20, 2016 16:13

kof reviewed Nov 20, 2016

View reviewed changes

cvle force-pushed the fix-failing-tests branch from 11d6055 to d1b0f65 Compare November 21, 2016 14:08

cvle force-pushed the fix-failing-tests branch from d1b0f65 to d16a568 Compare November 23, 2016 10:43

kof reviewed Nov 23, 2016

View reviewed changes

Fix failing tests across browsers

4850e5d

cvle force-pushed the fix-failing-tests branch from d16a568 to 4850e5d Compare November 23, 2016 11:32

kof merged commit 5afab46 into cssinjs:master Nov 23, 2016

AleshaOleg mentioned this pull request Oct 14, 2017

[jss-vendor-prefixer] Testing vendor prefixes cssinjs/jss#89

Closed

		@@ -1,2 +1,2 @@
		const context = require.context('./src', true, /\.test\.js$/)
		const context = require.context('./test', true, /\.test\.js$/)


		import {prefix, supportedProperty, supportedValue} from './index'

		console.log(`Detected browser: ${currentBrowser.id} ${currentBrowser.version}`)

Fix failing tests across browsers #9

Fix failing tests across browsers #9

Conversation

cvle commented Nov 20, 2016 • edited Loading

cvle commented Nov 20, 2016

cvle commented Nov 20, 2016

cvle commented Nov 20, 2016

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cvle commented Nov 20, 2016

kof commented Nov 20, 2016

cvle commented Nov 20, 2016

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

Choose a reason for hiding this comment

kof commented Nov 21, 2016

cvle commented Nov 21, 2016

cvle commented Nov 21, 2016

cvle commented Nov 21, 2016

kof commented Nov 23, 2016

Choose a reason for hiding this comment

cvle Nov 23, 2016 • 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

Choose a reason for hiding this comment

kof commented Nov 23, 2016

kof commented Nov 23, 2016

cvle commented Nov 23, 2016 • edited Loading

kof commented Nov 23, 2016

cvle commented Nov 23, 2016 • edited Loading

kof commented Nov 23, 2016

kof commented Nov 23, 2016

cvle commented Nov 23, 2016

kof commented Nov 23, 2016

cvle commented Nov 23, 2016 • edited Loading

cvle commented Nov 23, 2016

kof commented Nov 23, 2016

kof commented Nov 23, 2016

cvle commented Nov 23, 2016

kof commented Nov 23, 2016

cvle commented Nov 23, 2016

cvle commented Nov 23, 2016 • edited Loading

cvle commented Nov 20, 2016 •

edited

Loading

cvle Nov 23, 2016 •

edited

Loading

cvle commented Nov 23, 2016 •

edited

Loading

cvle commented Nov 23, 2016 •

edited

Loading

cvle commented Nov 23, 2016 •

edited

Loading

cvle commented Nov 23, 2016 •

edited

Loading