[WIP] Fix how we call grunt with multiple tasks #7461

ycombinator · 2016-06-15T07:14:14Z

This way, if we invoke npm test with options like npm test -- --esvm-no-fresh, the options are available to all grunt tasks, not just the last one.

This way, if we invoke `npm test` with options like `npm test -- --esvm-no-fresh`, the options are available to all grunt tasks, not just the last one.

ycombinator · 2016-06-15T07:56:09Z

jenkins, test it

jbudz · 2016-06-15T14:31:08Z

Any reason for not adding this to the grunt test command? Not running on jenkins? https://github.com/elastic/kibana/blob/master/tasks/test.js#L18

cjcenizal · 2016-06-15T14:31:41Z

👍 Awesome! Thanks for the improvement.

ycombinator · 2016-06-15T14:35:10Z

@jbudz I see no reason not to do it that way. I didn't know the original intent of doing it in package.json so I left it that way and made a minor improvement. @cjcenizal would it be okay to move test:visualRegression as a sub-task of test:quick in the grunt task file instead?

cjcenizal · 2016-06-15T15:11:27Z

@ycombinator After talking about it with @spalger, I put the task into package.json so that the visualRegression task would run regardless of the tests failing. I think if it's a sub-task, grunt will exit after a test fails, and visualRegression won't run. The long-term goal is to be able to run this on Jenkins and push the resulting visual regression gallery to S3. But I haven't been able to get access to Jenkins's configuration yet, so I haven't been able to set that up.

ycombinator · 2016-06-15T15:21:03Z

@cjcenizal I see. Then I think we need a way to accomplish both these constraints: run the visualRegression task regardless of tests failing and be able to pass any command-line options to all grunt invocations. Perhaps the solution lies in moving visualRegression into the grunt tasks file but not as a sub-task of test:quick.

spalger · 2016-06-15T20:16:33Z

The reason this was done this way is that we want the test:visualRegression task to run even if the test task fails. using --force won't work because it won't differentiate between the sub-tasks of test (which is shouldn't run if test fails) so the only way I can think of to do this is using the ; command separator.

ycombinator · 2016-06-15T20:25:22Z

@spalger The current behavior with using the ; separator seems broken to me. My specific use case is that I want to be able to call npm run test -- --esvm-no-fresh and not have ESVM download a fresh Elasticsearch build. Currently that's broken because the effective command becomes grunt test; grunt test:visualRegression --esvm-no-fresh. Note that the first grunt command never gets passed the --esvm-no-fresh option so it ends up downloading a fresh Elasticsearch build anyway.

I understand we want the test:visualRegression task to run even if the test task fails. But I also think any commandline options should be available to both tasks.

spalger · 2016-06-15T20:27:40Z

Yeah, that's fair, which is why I didn't close the pr :) we just can't merge it as is

ycombinator · 2016-06-15T20:28:17Z

Agreed. I'll mark it as WIP and remove the review label so its clearer that it should not be merged in its current state.

ycombinator · 2016-06-16T16:20:26Z

I looked into using grunt-continue but that simply toggles --force around specific tasks via continue:on and continue:off, which isn't going to cut it as Spencer noted in an earlier comment. Continuing to look into other ways of achieving both constraints.

epixa · 2016-06-16T18:16:16Z

I don't understand. Why not just add visualRegression to test?

… of them

ycombinator · 2016-06-16T20:13:54Z

@epixa Unless I'm misunderstanding your question, I think the answer is in this comment from CJ above: #7461 (comment).

BTW, I think I have something that works while satisfying both constraints. @spalger and others, mind giving this another review?

ycombinator · 2016-06-16T20:16:40Z

package.json

@@ -38,7 +38,7 @@
    "Tim Sullivan <tim@elastic.co>"
  ],
  "scripts": {
-    "test": "grunt test; grunt test:visualRegression",
+    "test": "bash -c 'grunt test $@; grunt test:visualRegression $@' --",


This does mean development has to be done on a system with bash on it, which may be too restrictive.

Yeah, that's a no go. The dev tasks need to be able to run on windows and such.

Okie doke. Marking this PR as WIP again.

epixa · 2016-06-16T20:17:46Z

Our tests don't bail once a single failure is found, so I'm not sure how adding visualRegression to grunt test would prevent it from running if there were tests failures.

cjcenizal · 2016-06-16T21:38:59Z

@epixa I did an experiment by adding the visualRegression task to test:ui:

grunt.registerTask('test:ui', [
  'esvm:ui',
  'run:testUIServer',
  'downloadSelenium',
  'run:seleniumServer',
  'clean:screenshots',
  'intern:dev',
  'test:visualRegression',
  'esvm_shutdown:ui',
  'stop:seleniumServer',
  'stop:testUIServer'
]);

I then hard-coded a test to fail and tried running grunt test. The intern:dev completes and reports the failure, but then grunt exits without running test:visualRegression and subsequent tasks.

epixa · 2016-06-16T21:51:44Z

In that case, if tests do bail once one task fails, why do we want visualRegression to behave differently than the other test suites?

cjcenizal · 2016-06-16T22:00:11Z

I think the idea is that we could use the generated screenshots/visual_regression_gallery.html to do some debugging. So you could run your tests, take a look at the gallery, and notice some UI error that's breaking the tests. At least that's the theory. 😄 I don't think the value of this has been proven yet...

cjcenizal · 2016-06-16T22:03:47Z

If this is blocking people or costing an inordinate amount of attention, I suggest we remove the visualRegression task from the npm scripts and refrain from adding it to the grunt test tasks. At this point, I think I'm the only person really using it, so I'm fine with just running grunt visualRegression locally for now.

epixa · 2016-06-16T22:06:03Z

For consistency's sake, we should just toss it at the end of grunt test and be done with it. You can always add a test:visualRegressions npm script to explicitly run this test if that's desired.

ycombinator · 2016-06-17T01:03:32Z

@cjcenizal Based on #7461 (comment), I'm going to close this PR (unmerged) so you can do a separate, clean PR instead.

`v93.3.0`⏩ `v93.4.0` --- ## [`v93.4.0`](https://github.com/elastic/eui/releases/v93.4.0) - Added the following properties to `EuiButtonGroup`'s `options` configs: `toolTipContent`, `toolTipProps`, and `title`. These new properties allow wrapping buttons in `EuiToolTips`, and additionally customizing or disabling the native browser `title` tooltip. ([#7461](elastic/eui#7461)) - Enhanced `EuiResizeObserver` and `useResizeObserver`'s performance to not trigger page reflows on resize event ([#7575](elastic/eui#7575)) - Updated `EuiSuperUpdateButton` to support custom button text via an optional `children` prop ([#7576](elastic/eui#7576)) **Bug fixes** - Fixed `EuiFlyout` to not repeatedly remove/add a body class on resize ([#7462](elastic/eui#7462)) - Fixed `EuiToast` title text to wrap instead of overflowing out of the container ([#7568](elastic/eui#7568)) - Fixed a visual bug with `EuiHeaderBreadcrumbs` with popovers ([#7580](elastic/eui#7580)) **Deprecations** - Deprecated `euiPalettePositive` and `euiPaletteNegative` in favour of a more culturally inclusive `euiPaletteGreen` and `euiPaletteRed` ([#7570](elastic/eui#7570)) - Deprecated all charts theme exports in favor of `@elastic/charts` exports: ([#7572](elastic/eui#7572)) - Deprecated `EUI_CHARTS_THEME_<DARK|LIGHT>` in favor of `<DARK|LIGHT>_THEME` from `@elastic/charts`. ([#7572](elastic/eui#7572)) - Deprecated `EUI_SPARKLINE_THEME_PARTIAL` in favor of `useSparklineOverrides` theme from the kibana `charts` plugin `theme` service. **Accessibility** - Updated `EuiModal` to set an `aria-modal` attribute and a default `dialog` role ([#7564](elastic/eui#7564)) - Updated `EuiConfirmModal` to set a default `alertdialog` role ([#7564](elastic/eui#7564)) - Fixed `EuiModal` and `EuiConfirmModal` to properly trap Safari+VoiceOver's virtual cursor ([#7564](elastic/eui#7564))

Fix how we call grunt with multiple tasks

1a459bb

This way, if we invoke `npm test` with options like `npm test -- --esvm-no-fresh`, the options are available to all grunt tasks, not just the last one.

ycombinator added the review label Jun 15, 2016

ycombinator assigned cjcenizal Jun 15, 2016

ycombinator removed the review label Jun 15, 2016

ycombinator changed the title ~~Fix how we call grunt with multiple tasks~~ [WIP] Fix how we call grunt with multiple tasks Jun 15, 2016

ycombinator unassigned cjcenizal Jun 15, 2016

Wrap both grunt commands in shell command so we can pass args to each…

430d70d

… of them

ycombinator added the review label Jun 16, 2016

ycombinator changed the title ~~[WIP] Fix how we call grunt with multiple tasks~~ Fix how we call grunt with multiple tasks Jun 16, 2016

ycombinator reviewed Jun 16, 2016
View reviewed changes

ycombinator removed the review label Jun 16, 2016

ycombinator changed the title ~~Fix how we call grunt with multiple tasks~~ [WIP] Fix how we call grunt with multiple tasks Jun 16, 2016

ycombinator closed this Jun 17, 2016

ycombinator deleted the grunt-multi-task branch June 17, 2016 01:03

cjcenizal mentioned this pull request Jun 20, 2016

Move visualRegression subtask from npm scripts to grunt task. #7515

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Fix how we call grunt with multiple tasks #7461

[WIP] Fix how we call grunt with multiple tasks #7461

ycombinator commented Jun 15, 2016

ycombinator commented Jun 15, 2016

jbudz commented Jun 15, 2016 •

edited

Loading

cjcenizal commented Jun 15, 2016

ycombinator commented Jun 15, 2016

cjcenizal commented Jun 15, 2016

ycombinator commented Jun 15, 2016 •

edited

Loading

spalger commented Jun 15, 2016 •

edited

Loading

ycombinator commented Jun 15, 2016 •

edited

Loading

spalger commented Jun 15, 2016

ycombinator commented Jun 15, 2016

ycombinator commented Jun 16, 2016 •

edited

Loading

epixa commented Jun 16, 2016

ycombinator commented Jun 16, 2016

ycombinator Jun 16, 2016

epixa Jun 16, 2016

ycombinator Jun 16, 2016

epixa commented Jun 16, 2016

cjcenizal commented Jun 16, 2016 •

edited

Loading

epixa commented Jun 16, 2016

cjcenizal commented Jun 16, 2016

cjcenizal commented Jun 16, 2016 •

edited

Loading

epixa commented Jun 16, 2016

ycombinator commented Jun 17, 2016

[WIP] Fix how we call grunt with multiple tasks #7461

[WIP] Fix how we call grunt with multiple tasks #7461

Conversation

ycombinator commented Jun 15, 2016

ycombinator commented Jun 15, 2016

jbudz commented Jun 15, 2016 • edited Loading

cjcenizal commented Jun 15, 2016

ycombinator commented Jun 15, 2016

cjcenizal commented Jun 15, 2016

ycombinator commented Jun 15, 2016 • edited Loading

spalger commented Jun 15, 2016 • edited Loading

ycombinator commented Jun 15, 2016 • edited Loading

spalger commented Jun 15, 2016

ycombinator commented Jun 15, 2016

ycombinator commented Jun 16, 2016 • edited Loading

epixa commented Jun 16, 2016

ycombinator commented Jun 16, 2016

ycombinator Jun 16, 2016

Choose a reason for hiding this comment

epixa Jun 16, 2016

Choose a reason for hiding this comment

ycombinator Jun 16, 2016

Choose a reason for hiding this comment

epixa commented Jun 16, 2016

cjcenizal commented Jun 16, 2016 • edited Loading

epixa commented Jun 16, 2016

cjcenizal commented Jun 16, 2016

cjcenizal commented Jun 16, 2016 • edited Loading

epixa commented Jun 16, 2016

ycombinator commented Jun 17, 2016

jbudz commented Jun 15, 2016 •

edited

Loading

ycombinator commented Jun 15, 2016 •

edited

Loading

spalger commented Jun 15, 2016 •

edited

Loading

ycombinator commented Jun 15, 2016 •

edited

Loading

ycombinator commented Jun 16, 2016 •

edited

Loading

cjcenizal commented Jun 16, 2016 •

edited

Loading

cjcenizal commented Jun 16, 2016 •

edited

Loading