Added JS Jasmine tests to Travis CI #9013

Igloczek · 2017-03-25T17:29:29Z

JS tests are already written, but not used on Travis CI pipeline, so I added the necessary config to enable them.

ishakhsuvarov · 2017-03-27T09:53:40Z

Hi @Igloczek
Thank you for this PR. It is really a great idea to add JS Jasmine tests to the Travis-ci.

Could you please look into some questions I've left in the comments? Thanks!

Igloczek · 2017-03-27T10:02:35Z

@ishakhsuvarov I'm pretty sure you forgot to hit "Submit review" 😉

ishakhsuvarov · 2017-03-27T09:50:49Z

dev/travis/js/config.php

@@ -0,0 +1,360 @@
+<?php


Adding a hard-coded configuration may be too fragile, since it may be a subject for modification. Please consider installing to get the generated file

I was following docs in this matter, b/c installing the whole application, just to get statics, it's serious overhead for me.

Maybe simpler option will be to work on source files, rather than processing them through M core, before running tests? It should be also easier for development purposes.

The documentation in question is about generating static on a non-production machine, so this is a slightly different case. Providing a hard-coded configuration file is not the best practice and needs to be maintained all the time.

I would still suggest installing application for now, at least for consistency, since internally this is what we do to run Jasmine tests.

@ishakhsuvarov could you move some internal code to public maybe so that we don't have two different implementations?

From https://github.com/magento/magento2/blob/develop/dev/travis/before_script.sh#L61 it looks it is not so easy to steal application installation from integration tests and it is placed somewhere inside framework.

As to me hardcoded config seems fine at least as a temporary solution if we add comment on top of it how to regenerate file.

@orlangur It is executed under different infrastructure then Travis CI. Would not solve the issue in question.
"Stealing" application from the integration tests is not a good solution either. Clean install should do the trick for now, while no better solution is available.

@ishakhsuvarov what do you mean by clean install? :) I thought installation in scope of integration tests is clean. Is it possible to install application without database at all, for example?

@ishakhsuvarov Could you share part of your tests configuration related to installation? Will be much easier to copy-paste(-adjust) working solution 😉

@Igloczek So, I've done some brief experiments with Travis CI and here is what seems to be working:

Instead of bundling config.php add the minimum installation as follows (it does not add too much time to the execution):

echo "Installing Magento"
mysql -uroot -e 'CREATE DATABASE magento2;'
php bin/magento setup:install -q --admin-user="admin" --admin-password="123123q" --admin-email="[email protected]" --admin-firstname="John" --admin-lastname="Doe"

You may do some experiments with the static content deployment settings to add a bit more performance. Here is what worked for me (may not be the best solution, just experiment):

echo "Deploying Static Content"
php bin/magento setup:static-content:deploy -f -q -j=2 --no-css --no-less --no-images --no-fonts --no-misc --no-html-minify

ishakhsuvarov · 2017-03-27T09:51:48Z

dev/travis/before_script.sh

+
+        cp package.json.sample package.json
+        cp Gruntfile.js.sample Gruntfile.js
+        npm install -g yarn


Is it really viable to install yarn just to install grunt-cli?

Not only grunt-cli, all dependencies defined in package.json are downloaded using yarn in line 90 (yarn is replacement for npm install).

Thanks for clarification.

ishakhsuvarov · 2017-03-27T10:05:51Z

@Igloczek You are so right

orlangur · 2017-03-27T10:25:55Z

dev/travis/before_script.sh

+        curl -o- https://raw.githubusercontent.com/creationix/nvm/v0.33.1/install.sh | bash
+        export NVM_DIR="$HOME/.nvm"
+        [ -s "$NVM_DIR/nvm.sh" ] && . "$NVM_DIR/nvm.sh" # This loads nvm
+        nvm install --lts


I believe it would be more robust to specify some concrete version, like in https://github.com/BanzaiMan/travis_production_test/blob/9c02aef/.travis.yml#L15, so that we see in environment variable which version we test against

I'm not sure if we really need to care about precise versioning, b/c in Oct 2017 we should be ready to use v7 without any issues and necessary updates in code base.

Node.js LTS plan is precisely described here - https://github.com/nodejs/LTS

If I understood the doc right v7 is not LTS and will not be used in such code.

The point is that I don't see any value in dynamic Node.js version, it can only bring us some headache when the LTS version will switch. Explicit is better than implicit.

As soon as v8 stable is released we can change value in our config if the tests still pass. Current version is like specifying php: stable version instead of concrete value.

You are right, v8, not v7.
LTS means more than stable, b/c stable v8 will be released in April 2017, but will be marked as LTS in Oct 2017. It's a different approach than PHP takes.

Is it guaranteed that using v6 for tests before, we will not have any issues under v8 when it become LTS? What's BC police?

OFC there are BC between major releases, but major becomes LTS after half year and most of the packages get updates during this period, so migration is in most of the cases are seamless.

It does not mean all our tests will work without changes.

Just answer the simple question - what is the value of NOT specifying a concrete version but using LTS instead?

By the way, package.json.sample bundled with Magento also has version requirement.

You are always up to date, don't have to follow releases schedule etc. When new LTS version will be up, your tests will start using it.

But to just quit discussion, I'll change it to 6 and this will be good enough till next LTS.

Docs about engines option from package.json

this field is advisory only will produce warnings when your package is installed as a dependency.

So in our case, we can simply remove this param.

Thanks!

When new LTS version will be up, your tests will start using it.

Yeah, that's a scary part as to me, as put our builds a bit out of control.

orlangur · 2017-03-27T10:28:46Z

.travis.yml

 before_install: ./dev/travis/before_install.sh
 install: composer install --no-interaction --prefer-dist
 before_script: ./dev/travis/before_script.sh
 script:
  - test $TEST_SUITE = "static" && TEST_FILTER='--filter "Magento\\Test\\Php\\LiveCodeTest"' || true
-  - phpunit -c dev/tests/$TEST_SUITE $TEST_FILTER
+  - if [ $TEST_SUITE != "js" ]; then phpunit -c dev/tests/$TEST_SUITE $TEST_FILTER; fi
+  - if [ $TEST_SUITE == "js" ]; then grunt spec; fi


Looking at https://travis-ci.org/magento/magento2/jobs/215020317 - is there some less verbose mode? Display only failed tests maybe...

No idea, for now I just focuses on make this test running, then we can think about some adjustments / improvements.

PS. I really hate Grunt, so if I don't have to modify this, I will be more than happy to leave it as it is 😉

Ok, I didn't use it as well, and didn't google for a solution just in case you know the answer.

It can wait until it become a problem "output is too big" some day (hopefully never).

ishakhsuvarov · 2017-03-28T10:24:07Z

@Igloczek Looks good to me now! Thanks for the fixes.
I am going to restart Travis build, to get rid of the Some checks were not successful error, and proceed with the merge.

Igloczek · 2017-03-28T10:27:14Z

You are faster than my thoughts, I was going to do same thing 😄

Igloczek · 2017-03-28T17:23:53Z

Please stop the merging process, I found a bug related to NVM and we are now using some built-in Node.js version.

ishakhsuvarov · 2017-03-28T17:27:39Z

@Igloczek Thanks for reporting

Igloczek · 2017-03-28T18:11:16Z

Fixed.

orlangur · 2017-03-28T20:22:08Z

dev/travis/before_script.sh

@@ -80,8 +80,8 @@ case $TEST_SUITE in
        curl -o- https://raw.githubusercontent.com/creationix/nvm/v0.33.1/install.sh | bash
        export NVM_DIR="$HOME/.nvm"
        [ -s "$NVM_DIR/nvm.sh" ] && . "$NVM_DIR/nvm.sh" # This loads nvm
-        bash -c "nvm install $NODE_JS_VERSION" || true


Why there was bash used at all in the first place?

Just b/c I'm a dumb copy-paster 🙄

That's absolutely fine as I had just an approximate understanding of what is going on when reviewed your code as well 👍

Hope we have some decent DevOps in Magento Community or in Magento Team to review Travis scripts :)

okorshenko · 2017-03-31T12:24:16Z

@Igloczek thank you for your contribution!

Igloczek added 13 commits March 20, 2017 08:51

Setup for runing JS Jasmine tests on Travis CI

384cebf

Fixed exlude option. Fixed before_install script.

9a23a98

NVM setup fix

0202a1a

NVM setup moved to before_script. NVM dir added to cache

7d7b388

NPM replaced with Yarn to speed up build process

9b779a7

bin/magento command debug

9db30d4

Fixed condition in Travis config file

14e6118

Local config added to deploy static view

b9011db

Environment variables filling

a0dd091

Solution working for 2.1.x

c28c7b4

Solution working for 2.2.x

39e06a3

Enable JS related setup steps

3434cb2

Re-enable default tests suite.

378a691

ishakhsuvarov reviewed Mar 27, 2017

View reviewed changes

okorshenko assigned ishakhsuvarov Mar 27, 2017

orlangur requested changes Mar 27, 2017

View reviewed changes

ishakhsuvarov added this to the March 2017 milestone Mar 27, 2017

ishakhsuvarov added the improvement label Mar 27, 2017

Igloczek added 4 commits March 28, 2017 12:09

More things added to cache #performance

dcdbe51

Unnecessary things removed from package.json

d21220a

Node version switched to 6.x

f078aff

Installing Magento instead using per-generated config.

d48e23f

ishakhsuvarov added the Progress: accept label Mar 28, 2017

ishakhsuvarov approved these changes Mar 28, 2017

View reviewed changes

orlangur approved these changes Mar 28, 2017

View reviewed changes

ishakhsuvarov added Progress: needs update and removed Progress: accept labels Mar 28, 2017

Get rid of bash commands.

a47530b

orlangur reviewed Mar 28, 2017

View reviewed changes

ishakhsuvarov added Progress: accept and removed Progress: needs update labels Mar 29, 2017

magento-team merged commit a47530b into magento:develop Mar 31, 2017

magento-team pushed a commit that referenced this pull request Mar 31, 2017

MAGETWO-66816: Added JS Jasmine tests to Travis CI #9013

1cd294d

Igloczek deleted the js-ci-test-on-travis branch April 1, 2017 07:32

Added JS Jasmine tests to Travis CI #9013

Added JS Jasmine tests to Travis CI #9013

Conversation

Igloczek commented Mar 25, 2017 • edited by ishakhsuvarov Loading

ishakhsuvarov commented Mar 27, 2017

Igloczek commented Mar 27, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Igloczek Mar 27, 2017 • edited Loading

Choose a reason for hiding this comment

ishakhsuvarov Mar 28, 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

ishakhsuvarov commented Mar 27, 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

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

ishakhsuvarov commented Mar 28, 2017

Igloczek commented Mar 28, 2017

Igloczek commented Mar 28, 2017

ishakhsuvarov commented Mar 28, 2017

Igloczek commented Mar 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okorshenko commented Mar 31, 2017

Igloczek commented Mar 25, 2017 •

edited by ishakhsuvarov

Loading

Igloczek Mar 27, 2017 •

edited

Loading

ishakhsuvarov Mar 28, 2017 •

edited

Loading