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

Tests: Add intern support #450

Open
wants to merge 52 commits into
from

Conversation

Projects
None yet
6 participants
@apsdehal
Contributor

apsdehal commented May 29, 2015

  • Shift all the tests to use QUnit interface by intern
  • Add grunt tasks for intern both for local and ci.
  • Modify deps according to the needs.

TODO:

  • Create separate Grunt tasks test:unit and test:functional
  • Create test:ci task and make sure it runs properly on Travis
  • Stop testing if any of the tests fails
  • Allow local browser testing using connect
  • Support all OSes and Browsers as mentioned in Globalize capabilities
  • Add es5-shim html test files
  • Support node matrix tests.
  • Include mobile browsers. #278
Show outdated Hide outdated test/intern.js
environments: [
{ browserName: "chrome" },
{ browserName: "internet explorer", version: [ "11", "10" ] },
{ browserName: "firefox", version: "34" }

This comment has been minimized.

This comment has been minimized.

@rxaviers

rxaviers May 30, 2015

Member

One additional comment: When testing IE8, we need to include ES5 shim (see unit-es5-shim.html and unit-es5-shim.html).

@rxaviers

rxaviers May 30, 2015

Member

One additional comment: When testing IE8, we need to include ES5 shim (see unit-es5-shim.html and unit-es5-shim.html).

Show outdated Hide outdated Gruntfile.js
serverOptions: serverOptions,
systemProperties: {}
}
}

This comment has been minimized.

@arschmitz

arschmitz May 30, 2015

might fault since it was not in the gist i referred you to but this should include

"stop-selenium-server": {
    dev: {}
},
@arschmitz

arschmitz May 30, 2015

might fault since it was not in the gist i referred you to but this should include

"stop-selenium-server": {
    dev: {}
},
Show outdated Hide outdated test/functional.js
@@ -1,5 +1,5 @@
require([

This comment has been minimized.

@rxaviers

rxaviers May 30, 2015

Member

Let's require([ "./functional/all" ], ...) given you have created it.

@rxaviers

rxaviers May 30, 2015

Member

Let's require([ "./functional/all" ], ...) given you have created it.

This comment has been minimized.

@apsdehal

apsdehal Jun 5, 2015

Contributor

Do you need functional.js and unit.js anymore? I don't think so.

@apsdehal

apsdehal Jun 5, 2015

Contributor

Do you need functional.js and unit.js anymore? I don't think so.

Show outdated Hide outdated test/functional/all.js
"test/functional/plural/plural",
"test/functional/relative-time/format-relative-time",
"test/functional/relative-time/relative-time-formatter"
]);

This comment has been minimized.

@rxaviers

rxaviers May 30, 2015

Member

Let's replace this list by the one from ./test/functional.js (including comments).

@rxaviers

rxaviers May 30, 2015

Member

Let's replace this list by the one from ./test/functional.js (including comments).

Show outdated Hide outdated test/unit/all.js
"test/unit/number/pattern-properties",
"test/unit/relative-time/format",
"test/unit/relative-time/properties"
]);

This comment has been minimized.

@rxaviers

rxaviers May 30, 2015

Member

Let's do the same changes I've pointed out above for test/functional.js and test/functional/all.js please.

@rxaviers

rxaviers May 30, 2015

Member

Let's do the same changes I've pointed out above for test/functional.js and test/functional/all.js please.

@rxaviers

This comment has been minimized.

Show comment
Hide comment
@rxaviers

rxaviers May 30, 2015

Member

The first time I ran grunt test-local, everything was executed just fine. Although, for the second and further times, I got the below (even having included stop-selenium-server that @arschmitz commented above [1]).

>> Error: Timeout waiting for selenium to start.  Check if an instance of selenium is already running.

After manually killing the live process, I was able to run it again.

$ ps aux | grep selenium
xavier   16619  1.7  2.1 4350708 84856 pts/19  Sl   07:04   0:03 java -jar node_modules/grunt-selenium-server/selenium-server-standalone-2.45.0.jar -Dwebdriver.chrome.driver=/tmp/globalize/node_modules/chromedriver/lib/chromedriver/chromedriver 

$ kill 16619

1:

diff --git a/Gruntfile.js b/Gruntfile.js
index 900c6a0..ee17768 100644
--- a/Gruntfile.js
+++ b/Gruntfile.js
@@ -477,6 +477,9 @@ module.exports = function( grunt ) {
                                        systemProperties: {}
                                }
                        }
+               },
+               "stop-selenium-server": {
+                       dev: {}
                }
        });
Member

rxaviers commented May 30, 2015

The first time I ran grunt test-local, everything was executed just fine. Although, for the second and further times, I got the below (even having included stop-selenium-server that @arschmitz commented above [1]).

>> Error: Timeout waiting for selenium to start.  Check if an instance of selenium is already running.

After manually killing the live process, I was able to run it again.

$ ps aux | grep selenium
xavier   16619  1.7  2.1 4350708 84856 pts/19  Sl   07:04   0:03 java -jar node_modules/grunt-selenium-server/selenium-server-standalone-2.45.0.jar -Dwebdriver.chrome.driver=/tmp/globalize/node_modules/chromedriver/lib/chromedriver/chromedriver 

$ kill 16619

1:

diff --git a/Gruntfile.js b/Gruntfile.js
index 900c6a0..ee17768 100644
--- a/Gruntfile.js
+++ b/Gruntfile.js
@@ -477,6 +477,9 @@ module.exports = function( grunt ) {
                                        systemProperties: {}
                                }
                        }
+               },
+               "stop-selenium-server": {
+                       dev: {}
                }
        });
@@ -480,10 +508,19 @@ module.exports = function( grunt ) {
// TODO fix issues, enable
// "jscs:dist",
"test:functional",

This comment has been minimized.

@rxaviers

rxaviers May 30, 2015

Member

There are two things I don't want to lose:

  1. Having local tests run on the default task.
  2. Having unit tests run before building (requirejs, ...) and having functional tests run before minification (uglify).
@rxaviers

rxaviers May 30, 2015

Member

There are two things I don't want to lose:

  1. Having local tests run on the default task.
  2. Having unit tests run before building (requirejs, ...) and having functional tests run before minification (uglify).
Show outdated Hide outdated test/intern.js
globalize: "dist/globalize",
json: "external/requirejs-plugins/src/json",
src: "src",
text: "external/requirejs-text/text"

This comment has been minimized.

@rxaviers

rxaviers May 30, 2015

Member

We should reuse (avoid duplicate) test/config.js.

@rxaviers

rxaviers May 30, 2015

Member

We should reuse (avoid duplicate) test/config.js.

This comment has been minimized.

@apsdehal

apsdehal Jun 5, 2015

Contributor

I am going to remove config.js, we don't need it anymore.

@apsdehal

apsdehal Jun 5, 2015

Contributor

I am going to remove config.js, we don't need it anymore.

@arschmitz

This comment has been minimized.

Show comment
Hide comment
@arschmitz

arschmitz May 30, 2015

@rxaviers ill checkout the stop selenium server part i asked him to add this i use it on other projects.

@rxaviers ill checkout the stop selenium server part i asked him to add this i use it on other projects.

@rxaviers

This comment has been minimized.

Show comment
Hide comment
@rxaviers

rxaviers May 30, 2015

Member

👍

Member

rxaviers commented May 30, 2015

👍

@apsdehal

This comment has been minimized.

Show comment
Hide comment
@apsdehal

apsdehal Jun 6, 2015

Contributor

It turns out intern doesn't support ie8 in unit tests, so I have removed it for now.

Contributor

apsdehal commented Jun 6, 2015

It turns out intern doesn't support ie8 in unit tests, so I have removed it for now.

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Jun 17, 2015

Contributor

@rxaviers are you happy with the latest changes? Do you want me to review this as well?

Contributor

jzaefferer commented Jun 17, 2015

@rxaviers are you happy with the latest changes? Do you want me to review this as well?

Show outdated Hide outdated Gruntfile.js
}
},
"stop-selenium-server": {
dev: {}

This comment has been minimized.

@rxaviers

rxaviers Jun 22, 2015

Member

Is this (selenium + chromedriver) a requirement for having tests to run on browserstack? Or is it a requirement for having tests to run on Chrome locally?

Isn't 'browserstack.selenium_version': '2.45.0' configuration sufficient? (more info).

@rxaviers

rxaviers Jun 22, 2015

Member

Is this (selenium + chromedriver) a requirement for having tests to run on browserstack? Or is it a requirement for having tests to run on Chrome locally?

Isn't 'browserstack.selenium_version': '2.45.0' configuration sufficient? (more info).

This comment has been minimized.

@arschmitz

arschmitz Jun 22, 2015

This is for running locally on chrome. Normally to run locally like this with intern you would need to download selenium, download, chromedriver start selenium with chrome driver then run the tests. This downloads everything with npm and starts and stops the server.

@arschmitz

arschmitz Jun 22, 2015

This is for running locally on chrome. Normally to run locally like this with intern you would need to download selenium, download, chromedriver start selenium with chrome driver then run the tests. This downloads everything with npm and starts and stops the server.

This comment has been minimized.

@rxaviers

rxaviers Jun 22, 2015

Member

Thanks @arschmitz. So, @apsdehal let's remove this change please. More details in (1.2) of this comment.

@rxaviers

rxaviers Jun 22, 2015

Member

Thanks @arschmitz. So, @apsdehal let's remove this change please. More details in (1.2) of this comment.

This comment has been minimized.

@arschmitz

arschmitz Jun 22, 2015

@rxaviers What your talking about in your comment below is rather different. This is for testing via webdriver locally it very much aids in contributors being able to do with with out the steps i list above browserstack is great but most contributors wont have access to this.

Intern supports 3 ways of running tests.

Via Selenium with webdriver
Via Node with a runner
Via the browser directly via client.html

I think contributors should be able to easily do all three of these locally for proper testing this addresses # 1 what you ask for in the comment below addresses # 3

@arschmitz

arschmitz Jun 22, 2015

@rxaviers What your talking about in your comment below is rather different. This is for testing via webdriver locally it very much aids in contributors being able to do with with out the steps i list above browserstack is great but most contributors wont have access to this.

Intern supports 3 ways of running tests.

Via Selenium with webdriver
Via Node with a runner
Via the browser directly via client.html

I think contributors should be able to easily do all three of these locally for proper testing this addresses # 1 what you ask for in the comment below addresses # 3

@rxaviers

This comment has been minimized.

Show comment
Hide comment
@rxaviers

rxaviers Jun 22, 2015

Member

@apsdehal you have done a great work so far. But, before continuing with my review, I think it would be good to align with you the goals I have in mind for testing below.

Do you think is it feasible to address them on this PR?

@apsdehal @arschmitz @jzaefferer Does anyone see anything missing in the below (or that could be performed in a better way)?


  1. Local testing

    1.1. Local Node.js

    Local Node.js testing is useful during development. They are fast to run and
    helps developer to know that unit and functional tests are passing for his local
    Node.js environment.

    The tests should be exposed via two Grunt tasks:

    • grunt test:unit for running local Node.js unit tests.
    • grunt test:functional for running local Node.js functional tests.

    These tests should be executed by CI. If any fails, there's no need to waste
    time running them on the various browsers (item 2 below).

    I think it's benefical changing what we currently do (run tests via phantomjs)
    and use intern instead due to its test coverage support. Sidenote: it's crazy
    thinking we currently run these tests via phantomjs instead of using plain
    Node.js.

    1.2. Local browser

    Local browser testing is useful during development. They allow for manual
    inspection on a specific browser.

    Developer should be able to run these tests by simply pointing any browser at:

    • http://localhost:9000/test/unit.html for unit tests.
    • http://localhost:9000/test/functional.html for functional tests.
    • http://localhost:9000/test/unit-es5-shim.html for unit tests (including es5 shim).
    • http://localhost:9000/test/functional-es5-shim.html for functional tests (including es5 shim).

    I see no need for changing what we currently have.

  2. Matrix testing
    2.1. Matrix Node.js testing

    Currently, this is missing. Can we have intern to execute the tests in a matrix of different Node.js environments (e.g., 0.10, 0.11, etc)?

    These tests should be exposed via a Grunt task:

    grunt nodeMatrixTest (better names are welcome)

    2.2. Matrix browsers and OSes testing

    Automated testing to ensure all browsers listed here (and the various OSes where they run on) are supported. I heard intern doesn't support automating the tests for IE8, which could be mitigated by us by running them manually (via 1.2).

    These tests should be exposed via a Grunt task:

    grunt browsersMatrixTest (better names are welcome)

    These tests should be executed by CI only in case if the faster grunt test (1.1) passes.

    I think running them through intern vs. browserstack is a great solution.

  3. Missing: CLDR matrix testing

    Globalize in theory works with any CLDR >= 26. But, we only test it using the latest CLDR. Can we automate CLDR matrix testing? I believe this is similar to 2.1.

Member

rxaviers commented Jun 22, 2015

@apsdehal you have done a great work so far. But, before continuing with my review, I think it would be good to align with you the goals I have in mind for testing below.

Do you think is it feasible to address them on this PR?

@apsdehal @arschmitz @jzaefferer Does anyone see anything missing in the below (or that could be performed in a better way)?


  1. Local testing

    1.1. Local Node.js

    Local Node.js testing is useful during development. They are fast to run and
    helps developer to know that unit and functional tests are passing for his local
    Node.js environment.

    The tests should be exposed via two Grunt tasks:

    • grunt test:unit for running local Node.js unit tests.
    • grunt test:functional for running local Node.js functional tests.

    These tests should be executed by CI. If any fails, there's no need to waste
    time running them on the various browsers (item 2 below).

    I think it's benefical changing what we currently do (run tests via phantomjs)
    and use intern instead due to its test coverage support. Sidenote: it's crazy
    thinking we currently run these tests via phantomjs instead of using plain
    Node.js.

    1.2. Local browser

    Local browser testing is useful during development. They allow for manual
    inspection on a specific browser.

    Developer should be able to run these tests by simply pointing any browser at:

    • http://localhost:9000/test/unit.html for unit tests.
    • http://localhost:9000/test/functional.html for functional tests.
    • http://localhost:9000/test/unit-es5-shim.html for unit tests (including es5 shim).
    • http://localhost:9000/test/functional-es5-shim.html for functional tests (including es5 shim).

    I see no need for changing what we currently have.

  2. Matrix testing
    2.1. Matrix Node.js testing

    Currently, this is missing. Can we have intern to execute the tests in a matrix of different Node.js environments (e.g., 0.10, 0.11, etc)?

    These tests should be exposed via a Grunt task:

    grunt nodeMatrixTest (better names are welcome)

    2.2. Matrix browsers and OSes testing

    Automated testing to ensure all browsers listed here (and the various OSes where they run on) are supported. I heard intern doesn't support automating the tests for IE8, which could be mitigated by us by running them manually (via 1.2).

    These tests should be exposed via a Grunt task:

    grunt browsersMatrixTest (better names are welcome)

    These tests should be executed by CI only in case if the faster grunt test (1.1) passes.

    I think running them through intern vs. browserstack is a great solution.

  3. Missing: CLDR matrix testing

    Globalize in theory works with any CLDR >= 26. But, we only test it using the latest CLDR. Can we automate CLDR matrix testing? I believe this is similar to 2.1.

@arschmitz

This comment has been minimized.

Show comment
Hide comment
@arschmitz

arschmitz Jun 22, 2015

@rxaviers

Local Node.js testing is useful during development. They are fast to run and
helps developer to know that unit and functional tests are passing for his local
Node.js environment.

This is part of intern already. Just need the tasks set up

Automated testing to ensure all browsers listed here (and the various OSes where they run on) are supported. I heard intern doesn't support automating the tests for IE8, which could be mitigated by us by running them manually (via 1.2).

Intern only supports IE8 for functional testing which you do not do ( or likely need ) with globalize there is no way manual or otherwise to run ie8 tests with intern. Chi does not support ie8 and is the assertion library used by intern. ES5 shim will not fix this.

How important do you consider testing against IE8 at this point it will no longer be supported in 6 months and you dont support it with out the use of es5 shim anyway. As a side not both ui and mobile have talked about the current versions we are getting ready to release being the last to support IE8.

There is intern geezer edition that supports ie8 but i don't think it supports qunit interface and has a lot of other disadvantages as well we may be able to use it just for ie8 though and add the interface for qunit i don't have any experience with geezer.

Developer should be able to run these tests by simply pointing any browser at: ...

It already works they are just redirects now all tests are run from a single html page within the node modules intern directory configuration options are passed to it to tell it what tests to run.

Matrix testing

The goal of this PR was just to really get globalize setup with intern for cross browser testing other sorts of matrix testing seem like they could be added later as you have time.

@rxaviers

Local Node.js testing is useful during development. They are fast to run and
helps developer to know that unit and functional tests are passing for his local
Node.js environment.

This is part of intern already. Just need the tasks set up

Automated testing to ensure all browsers listed here (and the various OSes where they run on) are supported. I heard intern doesn't support automating the tests for IE8, which could be mitigated by us by running them manually (via 1.2).

Intern only supports IE8 for functional testing which you do not do ( or likely need ) with globalize there is no way manual or otherwise to run ie8 tests with intern. Chi does not support ie8 and is the assertion library used by intern. ES5 shim will not fix this.

How important do you consider testing against IE8 at this point it will no longer be supported in 6 months and you dont support it with out the use of es5 shim anyway. As a side not both ui and mobile have talked about the current versions we are getting ready to release being the last to support IE8.

There is intern geezer edition that supports ie8 but i don't think it supports qunit interface and has a lot of other disadvantages as well we may be able to use it just for ie8 though and add the interface for qunit i don't have any experience with geezer.

Developer should be able to run these tests by simply pointing any browser at: ...

It already works they are just redirects now all tests are run from a single html page within the node modules intern directory configuration options are passed to it to tell it what tests to run.

Matrix testing

The goal of this PR was just to really get globalize setup with intern for cross browser testing other sorts of matrix testing seem like they could be added later as you have time.

@rxaviers

This comment has been minimized.

Show comment
Hide comment
@rxaviers

rxaviers Jun 22, 2015

Member

for functional testing which you do not do ( or likely need )

For clarification, yeap Globalize doesn't need the browser interaction simulation that intern calls functional. But, Globalize does have functional tests (i.e., tests that ensure the distributed built Globalize works as expected), which can run using what intern calls unit testing.

About IE8... Like I said, it can be tested manually in the meanwhile (while it's still supported) by using what I described in the item 1.2 above. I'm not concerned that we cannot automate its testing using intern.

It already works they are just redirects now all tests are run from a single html page within the node modules intern directory configuration options are passed to it to tell it what tests to run.

I want to keep things simple. In other words, introducing intern for 1.2 introduces an unnecessary complexity.

The goal of this PR was just to really get globalize setup with intern for cross browser testing other sorts of matrix testing seem like they could be added later as you have time.

I agree. Therefore, the items described in 2.1 and 3 could be ignored on this PR.

Member

rxaviers commented Jun 22, 2015

for functional testing which you do not do ( or likely need )

For clarification, yeap Globalize doesn't need the browser interaction simulation that intern calls functional. But, Globalize does have functional tests (i.e., tests that ensure the distributed built Globalize works as expected), which can run using what intern calls unit testing.

About IE8... Like I said, it can be tested manually in the meanwhile (while it's still supported) by using what I described in the item 1.2 above. I'm not concerned that we cannot automate its testing using intern.

It already works they are just redirects now all tests are run from a single html page within the node modules intern directory configuration options are passed to it to tell it what tests to run.

I want to keep things simple. In other words, introducing intern for 1.2 introduces an unnecessary complexity.

The goal of this PR was just to really get globalize setup with intern for cross browser testing other sorts of matrix testing seem like they could be added later as you have time.

I agree. Therefore, the items described in 2.1 and 3 could be ignored on this PR.

@arschmitz

This comment has been minimized.

Show comment
Hide comment
@arschmitz

arschmitz Jun 22, 2015

About IE8... Like I said, it can be tested manually in the meanwhile (while it's still supported) by using what I described in the item 1.2 above. I'm not concerned that we cannot automate its testing using intern.

Unless you are suggesting on keeping actual QUnit duplicating ALL the tests and having 2 complete and duplicated test setups im not sure what you mean by this?

I want to keep things simple. In other words, introducing intern for 1.2 introduces an unnecessary complexity.

What added complexity the fact that when you click the link it redirects?

About IE8... Like I said, it can be tested manually in the meanwhile (while it's still supported) by using what I described in the item 1.2 above. I'm not concerned that we cannot automate its testing using intern.

Unless you are suggesting on keeping actual QUnit duplicating ALL the tests and having 2 complete and duplicated test setups im not sure what you mean by this?

I want to keep things simple. In other words, introducing intern for 1.2 introduces an unnecessary complexity.

What added complexity the fact that when you click the link it redirects?

apsdehal added some commits Jun 5, 2015

Tests: Remove support for ie8
  - As intern doesn't support it
Tests: Fix intern's load timeout error
- Explicitly set intern/main in config paths
Tests: Enable node matrix testsing
- This will test globalize using phantom js in following envs:
	- 0.10
	- 0.11
	- 0.12
	- Travis Stable
- This will also test globalize on browsers in nodejs 0.10 version
Build: Add test:local task to Gruntfile
- `grunt test:local`
- Builds
- Runs connect server
- Navigate to `localhost:9000/test/unit.html`
- Navigate to `localhost:9000/test/functional.html`
@apsdehal

This comment has been minimized.

Show comment
Hide comment
@apsdehal

apsdehal Oct 11, 2015

Contributor

Fail fast option (to fail if any of tests fail) will take a little time.
theintern/intern#413

Contributor

apsdehal commented Oct 11, 2015

Fail fast option (to fail if any of tests fail) will take a little time.
theintern/intern#413

@apsdehal

This comment has been minimized.

Show comment
Hide comment
@apsdehal

apsdehal Oct 19, 2015

Contributor

@rxaviers This is ready to be tested in the current scenario, except the problem that I am not yet able to achieve 100% tests passing with browserstack because different things fail in varied scenarios on browserstack.

Contributor

apsdehal commented Oct 19, 2015

@rxaviers This is ready to be tested in the current scenario, except the problem that I am not yet able to achieve 100% tests passing with browserstack because different things fail in varied scenarios on browserstack.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment