Add checkPlugins task to check for plugins before running tests. by cjcenizal · Pull Request #8981 · elastic/kibana

cjcenizal · 2016-11-05T05:02:54Z

tylersmalley

Couple comments

tylersmalley · 2016-11-09T19:43:19Z

package.json

    "test:coverage": "grunt test:coverage",
    "test:visualRegression": "grunt test:visualRegression",
    "checkLicenses": "grunt licenses",
+    "checkPlugins": "grunt checkPlugins",


Since this is more of a dependency for other tasks, what do you think about removing the definition here?

tylersmalley · 2016-11-09T20:07:43Z

tasks/test.js

 module.exports = function (grunt) {
+  grunt.registerTask('checkPlugins', 'Checks for plugins which may disrupt tests', function checkPlugins() {
+    const done = this.async();
+    const pluginsDir = path.resolve('./plugins/');


I was hoping there was a better way to get this path as opposed to assembling it, but that doesn't appear to be the case.

Most of the folder paths are defined in Gruntfile.js, thoughts on moving this there?

tylersmalley · 2016-11-09T20:10:45Z

tasks/test.js

 const visualRegression = require('../utilities/visual_regression');

 module.exports = function (grunt) {
+  grunt.registerTask('checkPlugins', 'Checks for plugins which may disrupt tests', function checkPlugins() {


Since this is in the test task, we should namespace it to keep consistent with the rest of the tasks here. test:checkPlugins

(say test task 5 times fast)

Test task tes tast tesk tast testass tesp tasp. No sweat!

I was thinking that this task is like checkLicenses, which isn't name-spaced. They're alike in that they both validate some conditions of the code, but don't execute any of it... so they're not really tests, in that sense. I think we should leave the name-space off. Are you OK with that?

Fair enough

tylersmalley · 2016-11-09T20:14:58Z

tasks/test.js

+    const done = this.async();
+    const pluginsDir = path.resolve('./plugins/');
+
+    fs.readdir(pluginsDir, (err, files) => {


We should handle the error case as the message given isn't very helpful.

> kibana@6.0.0-alpha1 checkPlugins /Users/tyler/Code/tylersmalley/kibana-6 > grunt checkPlugins Running "checkPlugins" task Fatal error: Cannot read property 'filter' of undefined npm ERR! Darwin 15.6.0 npm ERR! argv "/Users/tyler/.nvm/versions/node/v6.9.0/bin/node" "/Users/tyler/.nvm/versions/node/v6.9.0/bin/npm" "run" "checkPlugins" npm ERR! node v6.9.0 npm ERR! npm v3.10.8 npm ERR! code ELIFECYCLE npm ERR! kibana@6.0.0-alpha1 checkPlugins: `grunt checkPlugins` npm ERR! Exit status 3 npm ERR! npm ERR! Failed at the kibana@6.0.0-alpha1 checkPlugins script 'grunt checkPlugins'. npm ERR! Make sure you have the latest version of node.js and npm installed. npm ERR! If you do, this is most likely a problem with the kibana package, npm ERR! not with npm itself. npm ERR! Tell the author that this fails on your system: npm ERR! grunt checkPlugins npm ERR! You can get information on how to open an issue for this project with: npm ERR! npm bugs kibana npm ERR! Or if that isn't available, you can get their info via: npm ERR! npm owner ls kibana npm ERR! There is likely additional logging output above. npm ERR! Please include the following file with any support request: npm ERR! /Users/tyler/Code/tylersmalley/kibana-6/npm-debug.log

tylersmalley · 2016-11-15T17:16:39Z

tasks/test.js


-  grunt.registerTask('test:server', [ 'esvm:test', 'simplemocha:all', 'esvm_shutdown:test' ]);
-  grunt.registerTask('test:browser', ['run:testServer', 'karma:unit']);
+  grunt.registerTask('test:server', [


👍 for line breaks

tylersmalley

LGTM

tylersmalley · 2016-11-15T17:17:20Z

@jbudz, mind being a second reviewer on this?

jbudz · 2016-11-15T20:24:58Z

As a heads up, there are external tests already running with plugins that use these tasks - there's a wip at #8626 for integration tests, and x-plugins does use npm run test:(browser/dev) with a custom plugin dir. /cc @LeeDr

jbudz · 2016-11-15T20:27:23Z

tasks/test.js

 const visualRegression = require('../utilities/visual_regression');

 module.exports = function (grunt) {
+  grunt.registerTask('checkPlugins', 'Checks for plugins which may disrupt tests', function checkPlugins() {


thoughts on moving this task to its own file?

cjcenizal · 2016-11-15T20:28:19Z

@jbudz When you say "plugins that use these tasks" which tasks are you referring to?

Backports PR #8981 **Commit 1:** Add checkPlugins task to check for plugins before running tests. * Original sha: 9e71c77 * Authored by CJ Cenizal <cj@cenizal.com> on 2016-11-05T05:02:13Z **Commit 2:** Remove checkPlugins npm script. Gracefully handle error case. * Original sha: 98ec182 * Authored by CJ Cenizal <cj@cenizal.com> on 2016-11-09T20:51:47Z **Commit 3:** Move checkPlugins task into its own module. * Original sha: bba7274 * Authored by CJ Cenizal <cj@cenizal.com> on 2016-11-15T23:40:41Z

…stic#9094) Backports PR elastic#8981 **Commit 1:** Add checkPlugins task to check for plugins before running tests. * Original sha: 9e71c77 * Authored by CJ Cenizal <cj@cenizal.com> on 2016-11-05T05:02:13Z **Commit 2:** Remove checkPlugins npm script. Gracefully handle error case. * Original sha: 98ec182 * Authored by CJ Cenizal <cj@cenizal.com> on 2016-11-09T20:51:47Z **Commit 3:** Move checkPlugins task into its own module. * Original sha: bba7274 * Authored by CJ Cenizal <cj@cenizal.com> on 2016-11-15T23:40:41Z Former-commit-id: 6d0cee0

cjcenizal added release_note:enhancement v5.1.0 v6.0.0 review labels Nov 5, 2016

Add checkPlugins task to check for plugins before running tests.

9e71c77

cjcenizal force-pushed the improvement/check-plugins-task branch from cd60ff6 to 9e71c77 Compare November 5, 2016 13:04

cjcenizal added the Team:Operations Kibana-Operations Team label Nov 5, 2016

cjcenizal assigned tylersmalley Nov 5, 2016

tylersmalley requested changes Nov 9, 2016

View reviewed changes

Remove checkPlugins npm script. Gracefully handle error case.

98ec182

tylersmalley reviewed Nov 15, 2016

View reviewed changes

tylersmalley approved these changes Nov 15, 2016

View reviewed changes

jbudz reviewed Nov 15, 2016

View reviewed changes

Move checkPlugins task into its own module.

bba7274

jbudz approved these changes Nov 15, 2016

View reviewed changes

cjcenizal merged commit 038a834 into elastic:master Nov 16, 2016

cjcenizal deleted the improvement/check-plugins-task branch November 16, 2016 00:11

elastic-jasper mentioned this pull request Nov 16, 2016

[5.x] Add checkPlugins task to check for plugins before running tests. #9094

Merged

ppisljar added the backported label Dec 2, 2016

epixa added v5.1.1 and removed v5.1.0 labels Dec 8, 2016

Comments

Conversation

cjcenizal commented Nov 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tylersmalley left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tylersmalley left a comment

Choose a reason for hiding this comment

Uh oh!

tylersmalley commented Nov 15, 2016

Uh oh!

jbudz commented Nov 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjcenizal commented Nov 15, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cjcenizal commented Nov 5, 2016 •

edited

Loading