Skip to content

fixing broken group bar chart#10313

Merged
ppisljar merged 3 commits intoelastic:masterfrom
ppisljar:fix/vislib-grouped-bars
Feb 15, 2017
Merged

fixing broken group bar chart#10313
ppisljar merged 3 commits intoelastic:masterfrom
ppisljar:fix/vislib-grouped-bars

Conversation

@ppisljar
Copy link
Copy Markdown
Contributor

@ppisljar ppisljar commented Feb 13, 2017

fixed error where "grouped" vertical bar charts Y-axis extents don't scale properly - fixes #10295

@ppisljar ppisljar added Feature:Visualizations Generic visualization features (in case no more specific feature label is available) review v5.3.0 v5.4.0 v6.0.0 labels Feb 13, 2017
@thomasneirynck thomasneirynck self-assigned this Feb 13, 2017
@ppisljar ppisljar force-pushed the fix/vislib-grouped-bars branch from 705aa97 to 684c4e1 Compare February 13, 2017 18:10
@thomasneirynck
Copy link
Copy Markdown
Contributor

jenkins, test this

@epixa epixa added the v5.2.2 label Feb 14, 2017
@epixa
Copy link
Copy Markdown
Contributor

epixa commented Feb 14, 2017

Let's fix this back to 5.2

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it would be better to break this test into two separate tests. One for should stack values when mode is stacked and the other for should stack values when mode is percentage. This way if one of them regresses, we can quickly identify which one.

@epixa
Copy link
Copy Markdown
Contributor

epixa commented Feb 15, 2017

jenkins, test this

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We shouldn't be writing tests this way: conditional assertions are bad news because if these tests were to never run, that should be a huge red flag for us, but we would never catch that with these tests. This all stems from the fact that these tests are all executed in a loop, which we shouldn't be doing either. Duplication across tests isn't something we should be programming our way around, it just makes us more likely to have bugs in the tests themselves, and it makes them harder to reason about, which is the absolute last thing we want from our tests. Plus, it tends to create problems like this conditional test stuff.

I'm OK with this test for the sake of getting this fix in, but please follow up with a new pull request that splits these tests out of the loop they're in and runs these stack/mode tests where appropriate without conditions.

@epixa
Copy link
Copy Markdown
Contributor

epixa commented Feb 15, 2017

If you rebase this on master, the flaky selenium failure should go away.

@ppisljar ppisljar force-pushed the fix/vislib-grouped-bars branch from 5bc2463 to 5d37d20 Compare February 15, 2017 15:02
@ppisljar
Copy link
Copy Markdown
Contributor Author

jenkins, test this

@ppisljar ppisljar merged commit c3c8e55 into elastic:master Feb 15, 2017
elastic-jasper added a commit that referenced this pull request Feb 15, 2017
Backports PR #10313

**Commit 1:**
fixing broken group bar chart

* Original sha: 37f3abe
* Authored by ppisljar <peter.pisljar@gmail.com> on 2017-02-13T16:21:05Z

**Commit 2:**
fixing test

* Original sha: 51aaecd
* Authored by ppisljar <peter.pisljar@gmail.com> on 2017-02-13T17:48:07Z

**Commit 3:**
updating based on last review

* Original sha: 5d37d20
* Authored by ppisljar <peter.pisljar@gmail.com> on 2017-02-15T09:15:08Z
elastic-jasper added a commit that referenced this pull request Feb 15, 2017
Backports PR #10313

**Commit 1:**
fixing broken group bar chart

* Original sha: 37f3abe
* Authored by ppisljar <peter.pisljar@gmail.com> on 2017-02-13T16:21:05Z

**Commit 2:**
fixing test

* Original sha: 51aaecd
* Authored by ppisljar <peter.pisljar@gmail.com> on 2017-02-13T17:48:07Z

**Commit 3:**
updating based on last review

* Original sha: 5d37d20
* Authored by ppisljar <peter.pisljar@gmail.com> on 2017-02-15T09:15:08Z
elastic-jasper added a commit that referenced this pull request Feb 15, 2017
Backports PR #10313

**Commit 1:**
fixing broken group bar chart

* Original sha: 37f3abe
* Authored by ppisljar <peter.pisljar@gmail.com> on 2017-02-13T16:21:05Z

**Commit 2:**
fixing test

* Original sha: 51aaecd
* Authored by ppisljar <peter.pisljar@gmail.com> on 2017-02-13T17:48:07Z

**Commit 3:**
updating based on last review

* Original sha: 5d37d20
* Authored by ppisljar <peter.pisljar@gmail.com> on 2017-02-15T09:15:08Z
ppisljar pushed a commit that referenced this pull request Feb 15, 2017
Backports PR #10313

**Commit 1:**
fixing broken group bar chart

* Original sha: 37f3abe
* Authored by ppisljar <peter.pisljar@gmail.com> on 2017-02-13T16:21:05Z

**Commit 2:**
fixing test

* Original sha: 51aaecd
* Authored by ppisljar <peter.pisljar@gmail.com> on 2017-02-13T17:48:07Z

**Commit 3:**
updating based on last review

* Original sha: 5d37d20
* Authored by ppisljar <peter.pisljar@gmail.com> on 2017-02-15T09:15:08Z
ppisljar pushed a commit that referenced this pull request Feb 15, 2017
Backports PR #10313

**Commit 1:**
fixing broken group bar chart

* Original sha: 37f3abe
* Authored by ppisljar <peter.pisljar@gmail.com> on 2017-02-13T16:21:05Z

**Commit 2:**
fixing test

* Original sha: 51aaecd
* Authored by ppisljar <peter.pisljar@gmail.com> on 2017-02-13T17:48:07Z

**Commit 3:**
updating based on last review

* Original sha: 5d37d20
* Authored by ppisljar <peter.pisljar@gmail.com> on 2017-02-15T09:15:08Z
ppisljar pushed a commit that referenced this pull request Feb 15, 2017
Backports PR #10313

**Commit 1:**
fixing broken group bar chart

* Original sha: 37f3abe
* Authored by ppisljar <peter.pisljar@gmail.com> on 2017-02-13T16:21:05Z

**Commit 2:**
fixing test

* Original sha: 51aaecd
* Authored by ppisljar <peter.pisljar@gmail.com> on 2017-02-13T17:48:07Z

**Commit 3:**
updating based on last review

* Original sha: 5d37d20
* Authored by ppisljar <peter.pisljar@gmail.com> on 2017-02-15T09:15:08Z
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature:Visualizations Generic visualization features (in case no more specific feature label is available) review v5.2.2 v5.3.0 v5.4.0 v6.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

"grouped" vertical bar charts Y-axis extents don't scale properly

3 participants