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

adding runPipeline tests #27015

Merged
merged 22 commits into from
Mar 21, 2019
Merged

adding runPipeline tests #27015

merged 22 commits into from
Mar 21, 2019

Conversation

ppisljar
Copy link
Member

@ppisljar ppisljar commented Dec 12, 2018

Summary

resolves #22139

  • as mentioned in the ticket, we would prefer to test against real es (as that would be hard to mock)
  • for testing quite some functions we actually need kibana running (as we need access to few kibana services which we would also prefer not to mock)

this implements runPipeline tests as selenium test:

  • with selenium tests we have es and kibana running
  • we create a simple plugin, which exposes the runPipeline wrapper on the window object
  • we use executeAsync inside selenium tests to run that function with parameters we provide

the result is a simple framework that allows us to test pipeline expressions with real es and kibana running.

  • its quite fast, once the page is loaded expressions can be executed in a breeze.
  • debugging is simple. if a test failed, copy the expression and run it in canvas.

in long term we probably want this to be more lightweight -- hopefully jest tests, but until we get there this could serve us well, and moving to jest once we have the infrastructure ready should not be a problem as we could implement a very similar wrapper there.

Running tests

node scripts/functional_tests_server --config test/interpreter_functional/config.js
node scripts/functional_test_runner --config test/interpreter_functional/config.js

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@ppisljar ppisljar added review WIP Work in progress v7.0.0 Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Dec 12, 2018
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@ppisljar
Copy link
Member Author

we are gonna try with a different approach

@ppisljar ppisljar closed this Dec 12, 2018
@ppisljar ppisljar reopened this Feb 28, 2019
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💔 Build Failed

@lukeelmers
Copy link
Member

Nice, this is looking good to me!

2 build failures: the first will be fixed with a rebase on master, and the second is screenshot flakiness.

Sounds like we just have to discuss what to do about those screenshots... #33187 is a related issue I ran into recently that would likely be solved by this too

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@stacey-gammon
Copy link
Contributor

Keep in mind we'll be getting screenshot tests with Applitools ( cc @liza-mae ). That might be a better route to verify the rendering and keep these tests just for testing the data output?

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@ppisljar
Copy link
Member Author

ppisljar commented Mar 20, 2019

I talked with spencer, he is ok with the partial screenshot approach, but suggested to use png resizing lib instead of canvas which requires native modules. I was suggested to use https://github.com/lovell/sharp and am giving it a try now.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@ppisljar
Copy link
Member Author

I tested new png cropping on windows and linux

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Member

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

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

LGTM! Tested and the new screenshots work great on OSX. Only caveat is that I don't know if the new library is really going to be helpful for maps, since it doesn't capture any of the tiles. But it certainly solves the problem for everything else.

I did notice a new console error from kbn_chrome.js, but after looking into it, I think it's likely due 4f0a57a, and the tests are still working either way.

TypeError: Cannot read property 'slice' of null
    at eval (kbn_chrome.js:80)
    at Scope.$broadcast (angular.js:18869)
    at eval (angular.js:14600)
    at Scope.$digest (angular.js:18352)
    at Scope.$apply (angular.js:18649)
    at bootstrapApply (angular.js:1958)
    at Object.invoke (angular.js:5106)
    at doBootstrap (angular.js:1956)
    at Object.bootstrap (angular.js:1976)
    at Object.chrome.bootstrap (chrome.js:96)

@ppisljar ppisljar merged commit fec0f6c into elastic:master Mar 21, 2019
ppisljar added a commit to ppisljar/kibana that referenced this pull request Mar 21, 2019
ppisljar added a commit to ppisljar/kibana that referenced this pull request Mar 21, 2019
ppisljar added a commit that referenced this pull request Mar 21, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Testing framework for new pipeline
4 participants