Merged
Conversation
Contributor
💚 Build Succeeded |
Author
|
New commit adds find.byClassName method instead of using remote service in add_panel. |
nreese
approved these changes
Aug 23, 2018
Contributor
nreese
left a comment
nreese
left a comment
There was a problem hiding this comment.
lgtm
code review, ran some functional tests that use dashboardAddPanel service. There is no longer a long pause when opening the add panel
Contributor
There was a problem hiding this comment.
I think this line and the line above (the commit about the timing issue) should just be removed
Author
There was a problem hiding this comment.
Yes, I'll remove that. I commented the sleep out in this PR hoping and expecting we wouldn't need it. I'm going to run this against Kibana in Cloud just to make sure it works there too.
Contributor
💚 Build Succeeded |
Contributor
💚 Build Succeeded |
LeeDr
pushed a commit
to LeeDr/kibana
that referenced
this pull request
Aug 24, 2018
* Rebuild modulePath correctly if on Windows * Change how we find dashboard add panel is loaded * Add and use find.byClassName instead of remote * cleanup a comment
LeeDr
pushed a commit
to LeeDr/kibana
that referenced
this pull request
Aug 24, 2018
* Rebuild modulePath correctly if on Windows * Change how we find dashboard add panel is loaded * Add and use find.byClassName instead of remote * cleanup a comment
LeeDr
pushed a commit
that referenced
this pull request
Aug 27, 2018
LeeDr
pushed a commit
that referenced
this pull request
Aug 27, 2018
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
UPDATE: Scratch everything I said below about this test;
"dashboard app using legacy data dashboard state Overriding colors on an area chart is preserved"
This PR will only speed up that dashboard add panel action.
The fix for the test that was in master and 6.x didn't really fix the problem. It still fails intermittently and will require a new fix.
I started working on fixing this test on 6.4.0;
"dashboard app using legacy data dashboard state Overriding colors on an area chart is preserved"
I found that when the test was clicking the dashboard Edit menu item, and then immediately opening the Count legend on the area chart that was in the dashboard, some tweak to that visualization to go into edit mode must cause the legend to get closed. So even though there was code to check that the legend was open, that check would pass, and then it would get closed before the next step of the test which was selecting a color.
While debugging this problem, I was bugged by a previous change I had implemented which waited for the dashboard Add Panel loading indicator to be deleted. So I investigated and have 2 potential solutions (see code change)
But as I tried to bring my change for the Dashboard Edit mode to master, I found that there's a recent change to this area from #21629 which didn't make it back to 6.4 branch (because the PR included more than we would want right before the 6.4.0 release).
So, this PR for master and 6.5 now only speeds up the dashboard tests.
But when I backport to 6.4 I would also include this change from the above PR to fix that dashboard "Overriding colors on an area chart" test.
https://github.com/elastic/kibana/pull/21629/files#diff-900adf3e48329f3b99bb703081058de3R176
The check below might not directly fix the problem, but since it takes at least some milliseconds to check, it could end up fixing the flaky test failure.
