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

Reenable Rollup Jobs API test that was failing due to interval change in ES #36310

Merged
merged 5 commits into from
May 9, 2019

Conversation

cjcenizal
Copy link
Contributor

@cjcenizal cjcenizal commented May 8, 2019

Fixes #36269

I did a quick run-through of the UI to verify things still work correctly. Rollup jobs look like they can be created and run just fine. Testing of Visualize was blocked by #36295

@cjcenizal cjcenizal added v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.2.0 Feature:Rollups labels May 8, 2019
@cjcenizal cjcenizal requested a review from jen-huang May 8, 2019 21:10
@elasticmachine
Copy link
Contributor

Pinging @elastic/es-ui

@jen-huang
Copy link
Contributor

I think we should also change the payload to use fixed_interval instead of interval. The test broke here because the interval: '24h' was coerced into fixed_interval based on the value. What do you think?

@jen-huang
Copy link
Contributor

jen-huang commented May 8, 2019

While you're in this code, do you think it would be a large effort to update the detail panel UI? The interval value is currently blank when the above coercion happens:

image

@cjcenizal
Copy link
Contributor Author

We discussed this in Slack and decided to leave the payload as-is, since that reflects the reality of what kind of payload is generated by the UI. The right time to update this test code is when we address #36306.

@cjcenizal
Copy link
Contributor Author

@jen-huang Great catch re the detail panel! I just pushed a fix. Could you take another look please?

image

image

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@@ -24,7 +24,7 @@ export default function ({ getService }) {
cleanUp,
} = registerHelpers({ supertest, es });

describe('jobs', () => {
describe('Rollup jobs', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This was written like this on purpose as each "describe" creates a section.
Its parent is "Rollup". So we would read the test like this: Rollup -> jobs -> Index patterns -> <should ...>.

@@ -139,7 +138,11 @@ export default function ({ getService }) {
'testCreatedField': {
'agg': 'date_histogram',
'delay': '1d',
'interval': '24h',
// TODO: Note that we created the job with `interval`, but ES has coerced this to
Copy link
Contributor

@sebelga sebelga May 9, 2019

Choose a reason for hiding this comment

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

Why don't we address it in this PR?

[edit]: sorry, just tested trying to provide "fixed_interval" in the payload and apparently it is not yet supported. So if I understand well, the create job API needs interval to be provided but the get job API returns fixed_interval?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

interval is deprecated, but still supported. When ES receives interval it will coerce it into either fixed_interval or calendar_interval, which is why we see one of those when getting the rollup capabilities/job from ES. Jen opened #36306 to update the UI at some point to support creation of a rollup job with either fixed_interval or calendar_interval, instead of interval, but it's not a necessary change at the moment.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@cuff-links
Copy link
Contributor

These tests run fine and passed for me locally. I also checked to make sure that we were checking some sad paths in our coverage. Good to go there.

@cuff-links cuff-links self-requested a review May 9, 2019 17:14
Copy link
Contributor

@cuff-links cuff-links left a comment

Choose a reason for hiding this comment

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

Looks really good. Left comment in convo. All passed locally.

@cjcenizal cjcenizal merged commit a3eaa8f into elastic:master May 9, 2019
@cjcenizal cjcenizal deleted the rollup/re-enable-api-test branch May 9, 2019 17:18
@cjcenizal cjcenizal changed the title Reenable Rollup Jobs API test. Reenable Rollup Jobs API test that was failing due to interval change in ES May 9, 2019
cjcenizal added a commit to cjcenizal/kibana that referenced this pull request May 9, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

cjcenizal added a commit to cjcenizal/kibana that referenced this pull request May 20, 2019
@cjcenizal cjcenizal added the non-issue Indicates to automation that a pull request should not appear in the release notes label May 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Rollups non-issue Indicates to automation that a pull request should not appear in the release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.2.0 v8.0.0
Projects
None yet
5 participants