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

add chart tests #44

Closed
wants to merge 5 commits into from
Closed

add chart tests #44

wants to merge 5 commits into from

Conversation

rlgomes
Copy link
Contributor

@rlgomes rlgomes commented Feb 18, 2016

fixes #43

basic set of chart tests for barchart, timechart and event overlays on timecharts.

@rlgomes
Copy link
Contributor Author

rlgomes commented Feb 18, 2016

@juttle/developers spamming everyone on this one since this does include everyone's work across the various components that make up juttle-engine. Right now just a quick set of tests to verify that barchart and timecharts work as expected within juttle-engine. The follow up PR for this will be to cover the other important charts we support and then ultimately start piecing together similar tests that start plugging in an adapter at a time using a docker container and verifying those produce the expected data through the whole pipeline and into a chart (even if to begin we simply dump to a text view to simply validate raw data).

@mattnibs
Copy link
Member

I think it's a good idea that we're doing integration tests, that said I think this particular suite of tests tends to focus on things that should be tested in juttle-viz (and are already test in juttle-viz). Running three tests the check for variations of options for barcharts really only tests stuff that juttle-viz should be testing. We could (more efficiently) test one a program that renders a barchart, timechart and table (with a couple mixed options) and we'd be covering a larger area of integration pieces:

  1. Does juttle-service properly pass view params to client?
  2. Does juttle-viewer parse the url set and fetch the proper program from juttle-service?
  3. Does juttle-client-library order views correctly from the program recieved from juttle-service?
  4. Is juttle-engine actually serving the juttle-viewer app?

My beliefs in summation- Only use browser based, selenium tests when you REALLY have to and sparingly as possible. Trust that the various repos are properly testing their isolated responsibilities.

@mattnibs
Copy link
Member

Also the adapter integration tests can be done without selenium and by calling the api directly.

@dmehra
Copy link
Contributor

dmehra commented Feb 18, 2016

We have established a goal of having end-to-end test coverage, so the conversation here is not about whether to have tests that read data in (via adapters, soon enough) and validate that we can see a chart - we will absolutely do so - only about which specific test cases to cover in e2e scenario vs at a lower level of integration tests.

@mnibecker can you point out which tests in this PR are unnecessary because they are already covered in juttle-viz or client library?

@mattnibs
Copy link
Member

w.r.t to testing juttle-client-library: These all test whether it can run a program against a live juttle-service and properly instantiate the appropriate juttle-view and pass it the correct option. We definitely need to have an integration test that this works, I suggested doing one with multiple charts. (we already unit tests the layout mechanism, client api's against a mock service, and job-manager api in jcl).

w.r.t to tests in juttle-viz: @go-oleg correct me if I'm wrong, but I don't think we test as a whole the charts spit out certain dom based on a given set of parameters. I do know we test individual generators (bars included). Not questioning whether these tests should exist, I'm just questioning whether the level of granularity might be more appropriate for juttle-viz.

@go-oleg
Copy link
Contributor

go-oleg commented Feb 19, 2016

Right, we don't inspect the DOM and verify that its correct, thats something that was done in the "system tests" in Jut 1.0.

@rlgomes rlgomes closed this Feb 23, 2016
@rlgomes rlgomes deleted the add-chart-tests branch February 23, 2016 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add integration tests for various charts supported by juttle-viz
4 participants