[Security Solution][Detections] Fix truncated values in columns for rules table #115825
Conversation
This file contains 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
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / general / X-Pack API Integration Tests.x-pack/test/api_integration/apis/security_solution/overview_network·ts.apis SecuritySolution Endpoints Overview Network With packetbeat Make sure that we get OverviewNetwork dataStandard OutStack TraceMetrics [docs]Async chunks
To update your PR or re-run it, just comment with: |
c23d519 to
d7d9b76
Compare
|
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
|
Pinging @elastic/security-solution (Team: SecuritySolution) |
xcrzx
reviewed
Oct 25, 2021
| test.each([ | ||
| ['Rules', rulesColumns], | ||
| ['Rule Monitoring', ruleMonitoringColumns], | ||
| ])('table "%s" should not have truncated text options for column items', (_, columns) => { |
There was a problem hiding this comment.
Do we need these tests? All they do is check config values. As soon as config values change, we'll need to update the tests as well. So they add some maintenance burden, but it doesn't look like they can catch an actual bug.
There was a problem hiding this comment.
If config value would change and no refactoring done to how certain cells are rendering(name, tags, etc), it will bring the issue again.
So, idea behind those tests was to help preventing the issue to come back and to make dev conscious about config changes(probably some additional comment to the tests might be helpful as well, let me know if it makes sense).
What do you think would be a better option? Remove tests altogether or add different ones?
There was a problem hiding this comment.
We have a business requirement, which (very simplified) sounds like: "Table content should be visible to users". And a test that verifies that the table config doesn't contain the truncateText value. I think this config value is a detail of the internal implementation of the table component. And the absence of the truncateText config property doesn't mean that the table content is fully visible. The property could be removed or renamed, or the table could start cropping every column, disregarding the config value. In all these cases, the test will still be green.
I think we should design our test in a way that helps us verify business requirements without knowing internal implementation details. And for this particular case, it could be a cypress test that checks table elements visibility or a visual regression test (but we don't have an infrastructure for visual regression testing afaik). But I don't think it's worth adding complicated test cases to cover this requirement, so just removing the current tests would also be acceptable.
There was a problem hiding this comment.
I don't have a super strong opinion on this, but @xcrzx 's thoughts make sense to me.
- For a developer who would jump into these tests it wouldn't be immediately clear why these tests are there and what behaviour/requirements they actually verify. Adding a link to the bug in a comment would help (please do if you decide to keep these tests).
- The tests seem to check internal implementation details, and there's a chance that some refactoring (in EUI or on our side) would break them.
- These tests don't 100% guarantee that the 2 tables actually use the values from
getColumnsandgetMonitoringColumns(unless there are tests for that specifically).
it could be a cypress test that checks table elements visibility
This could be an option, but Cypress sounds too heavy imho. I would check if there's a way to deeply render the full table in a unit test and verify that the content in all cells is not truncated. If that's possible.
Still I think it would be a relatively brittle test. UI requirements might change often, we might introduce new components that would render the content differently, etc.
or a visual regression test (but we don't have an infrastructure for visual regression testing afaik)
Imho this would be a preferable type of test in this case. But I haven't heard about visual regression testing for Kibana either.
There was a problem hiding this comment.
thanks for the meaningful feedback @banderror, @xcrzx.
I've removed tests.
I would check if there's a way to deeply render the full table in a unit test and verify that the content in all cells is not truncated. If that's possible.
I think, it's not. Jsdom doesn't load layout, and truncation happens within CSS. So either cypress or visual regression tests could handle such case.
I agree that these tests did not guarantee desired outcome. Just want to give you a bit of a thought process behind the decision. I basically treated getColumns as a public method of columns module, that exposes contract to third part component: EuiBasicTable. Ideally, only major version of @elastic/eui would've pose threat to break intended functionality. But unfortunately, these tests would've not be able to catch it anyway.
|
@elasticmachine merge upstream |
banderror
approved these changes
Oct 26, 2021
There was a problem hiding this comment.
LGTM 👍 Thank you for the fix and congrats with the first PR in Kibana ❤️
Left my 2 cents regarding the tests, but I'm ok with whatever decision you will come to with @xcrzx. If you decide to rework the tests, I'd suggest to do it in a separate PR - so we could merge this one before the next 7.16 BC (build candidate) which is planned for tomorrow (see https://github.com/elastic/dev/issues/1809).
| test.each([ | ||
| ['Rules', rulesColumns], | ||
| ['Rule Monitoring', ruleMonitoringColumns], | ||
| ])('table "%s" should not have truncated text options for column items', (_, columns) => { |
There was a problem hiding this comment.
I don't have a super strong opinion on this, but @xcrzx 's thoughts make sense to me.
- For a developer who would jump into these tests it wouldn't be immediately clear why these tests are there and what behaviour/requirements they actually verify. Adding a link to the bug in a comment would help (please do if you decide to keep these tests).
- The tests seem to check internal implementation details, and there's a chance that some refactoring (in EUI or on our side) would break them.
- These tests don't 100% guarantee that the 2 tables actually use the values from
getColumnsandgetMonitoringColumns(unless there are tests for that specifically).
it could be a cypress test that checks table elements visibility
This could be an option, but Cypress sounds too heavy imho. I would check if there's a way to deeply render the full table in a unit test and verify that the content in all cells is not truncated. If that's possible.
Still I think it would be a relatively brittle test. UI requirements might change often, we might introduce new components that would render the content differently, etc.
or a visual regression test (but we don't have an infrastructure for visual regression testing afaik)
Imho this would be a preferable type of test in this case. But I haven't heard about visual regression testing for Kibana either.
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: cc @vitaliidm |
|
The following labels were identified as gaps in your version labels and will be added automatically:
If any of these should not be on your pull request, please manually remove them. |
kibanamachine
added a commit
to kibanamachine/kibana
that referenced
this pull request
Oct 29, 2021
…ules table (elastic#115825) * Fix truncated rule name on rules table * remove empty line * remove truncateText for all rules table columns * rename unit tests * refactor unit tests * fix ts error * fix ts error * remove config line * fix types after kbn bootstrap * CR: remove unnecesary tests Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
kibanamachine
added a commit
to kibanamachine/kibana
that referenced
this pull request
Oct 29, 2021
…ules table (elastic#115825) * Fix truncated rule name on rules table * remove empty line * remove truncateText for all rules table columns * rename unit tests * refactor unit tests * fix ts error * fix ts error * remove config line * fix types after kbn bootstrap * CR: remove unnecesary tests Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
|
Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync. |
kibanamachine
added a commit
that referenced
this pull request
Nov 1, 2021
…ules table (#115825) (#116795) * Fix truncated rule name on rules table * remove empty line * remove truncateText for all rules table columns * rename unit tests * refactor unit tests * fix ts error * fix ts error * remove config line * fix types after kbn bootstrap * CR: remove unnecesary tests Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Vitalii Dmyterko <92328789+vitaliidm@users.noreply.github.com>
kibanamachine
added a commit
that referenced
this pull request
Nov 2, 2021
…ules table (#115825) (#116796) * Fix truncated rule name on rules table * remove empty line * remove truncateText for all rules table columns * rename unit tests * refactor unit tests * fix ts error * fix ts error * remove config line * fix types after kbn bootstrap * CR: remove unnecesary tests Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Vitalii Dmyterko <92328789+vitaliidm@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Ticket 113607
Summary
This PR fixes issue where column text was truncated, and no tooltip displayed to see full text.
In PR table behaviour is restored to the behaviour as in 7.15 version(wrapping fields, rather than truncating)
UI Before
UI After
Checklist
Delete any items that are not applicable to this PR.
Risk Matrix