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

JS Static tests added to CI (ESLint + JSCS) #9093

Merged
merged 10 commits into from May 16, 2017
5 changes: 1 addition & 4 deletions .travis.yml
Expand Up @@ -39,7 +39,4 @@ matrix:
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
- if [ $TEST_SUITE != "js" ]; then phpunit -c dev/tests/$TEST_SUITE $TEST_FILTER; fi
- if [ $TEST_SUITE == "js" ]; then grunt spec; fi
script: ./dev/travis/script.sh
27 changes: 0 additions & 27 deletions dev/tools/grunt/configs/eslint.js

This file was deleted.

18 changes: 18 additions & 0 deletions dev/tools/grunt/configs/eslint.json
@@ -0,0 +1,18 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, convert jscs config to json too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

"file": {
"options": {
"configFile": "dev/tests/static/testsuite/Magento/Test/Js/_files/eslint/.eslintrc",
"reset": true,
"useEslintrc": false
}
},
"test": {
"options": {
"configFile": "dev/tests/static/testsuite/Magento/Test/Js/_files/eslint/.eslintrc",
"reset": true,
"outputFile": "dev/tests/static/eslint-error-report.xml",
"format": "junit",
"quiet": true
}
}
}
8 changes: 6 additions & 2 deletions dev/tools/grunt/tasks/static.js
Expand Up @@ -17,7 +17,7 @@ module.exports = function (grunt) {
};

grunt.registerTask('static', function (target) {
var currentTarget = target || 'test',
var currentTarget = target || 'file',
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently we use grunt static to run all test with generating log file and grunt static:file --file=<path_to_file> for single file validation. What reason to switch this?

Copy link
Contributor Author

@Igloczek Igloczek Apr 14, 2017

Choose a reason for hiding this comment

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

UX. Dev should be able to quickly run grunt static to get some instant, readable feedback, without trying to find undocumented tasks options.

If you need to generate this XML files for some reason, run gulp static:test instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, make sense. But change, please, in dev/travis/script.sh grunt static to grunt static:test.

Copy link
Contributor Author

@Igloczek Igloczek May 2, 2017

Choose a reason for hiding this comment

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

But... the whole point is to not use test on CI, b/c output is saved only as JUnit XML file, which isn't available, b/c build artifacts are not saved anywhere (even if they will be publicly available, it's way more complicated way than just read the tests output).

Also, quiet option do more harm than good, b/c ignore all warnings.

Results of gulp static:test

Running "eslint:test" (eslint) task
Warning: Task "eslint:test" failed. Used --force, continuing.

Results of gulp static aka. gulp static:file

Running "eslint:file" (eslint) task
/Users/igloczek/Sites/magento2-dev/lib/web/mage/accordion.js
  7:9  error  Parsing error: Unexpected token 'jquery'

✖ 1 problem (1 error, 0 warnings)

Warning: Task "eslint:file" failed. Used --force, continuing.

file = grunt.option('file'),
tasks = [
'eslint:' + currentTarget,
Expand All @@ -26,7 +26,11 @@ module.exports = function (grunt) {

setConfig('eslint', currentTarget, cvf.getFiles(file));
setConfig('jscs', currentTarget, cvf.getFiles(file));
grunt.option('force', true);

if (grunt.option('force') !== false) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, revert this changes, we already solve this issue. We add grunt-continue, that provide ability to run in force mode(for collect eslint and jscs errors) and fail on warnings. Internal ticket MAGETWO-67342 now in delivery process.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

grunt.option('force', false);
}

grunt.task.run(tasks);

if (!grunt.option('file')) {
Expand Down
14 changes: 14 additions & 0 deletions dev/travis/before_install.sh
Expand Up @@ -19,3 +19,17 @@ phpenv rehash;

# If env var is present, configure support for 3rd party builds which include private dependencies
test -n "$GITHUB_TOKEN" && composer config github-oauth.github.com "$GITHUB_TOKEN" || true

# Node.js setup via NVM
if [ $TEST_SUITE = "static" ] || [ test $TEST_SUITE == "js" ]; then
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 $NODE_JS_VERSION
nvm use $NODE_JS_VERSION
node --version

npm install -g yarn
yarn global add grunt-cli
fi
13 changes: 4 additions & 9 deletions dev/travis/before_script.sh
Expand Up @@ -75,19 +75,14 @@ case $TEST_SUITE in
cat "$changed_files_ce" | sed 's/^/ + including /'

cd ../../..

cp package.json.sample package.json
cp Gruntfile.js.sample Gruntfile.js
yarn
;;
js)
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 $NODE_JS_VERSION
nvm use $NODE_JS_VERSION
node --version

cp package.json.sample package.json
cp Gruntfile.js.sample Gruntfile.js
npm install -g yarn
yarn global add grunt-cli
yarn

echo "Installing Magento"
Expand Down
10 changes: 10 additions & 0 deletions dev/travis/script.sh
@@ -0,0 +1,10 @@
case $TEST_SUITE in
static)
TEST_FILTER='--filter "Magento\\Test\\Php\\LiveCodeTest"' || true
phpunit -c dev/tests/$TEST_SUITE $TEST_FILTER
grunt static --force=false
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, remove --force.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

;;
js)
grunt spec
;;
esac