-
Notifications
You must be signed in to change notification settings - Fork 113
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 Discover Table Tests #1182
Add Discover Table Tests #1182
Conversation
cy.waitForLoader(); | ||
}); | ||
|
||
after(() => {}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Hailong-am Removed it in the latest version of the PR.
// Setting up the page | ||
describe('discover_table', () => { | ||
beforeEach(() => { | ||
if (Cypress.env('SECURITY_ENABLED')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need this if (Cypress.env('SECURITY_ENABLED')) {
since this path cypress/integration/core-opensearch-dashboards/opensearch-dashboards/apps/data_explorer
is different from cypress/integration/core-opensearch-dashboards/opensearch-dashboards/dashboard_share_copy_link_test.js
which will be tested in component and Integ in release pipeline. Most security related ones are in cypress/integration/plugins/security
. For data_explorer, we don't need to test security.
Seems our current tests are not consistency. I see one test has check this cypress/integration/core-opensearch-dashboards/opensearch-dashboards/apps/data_explorer/discover.spec.js
but all the rest don't. Maybe we should clean this one as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated both the tests to remove the check specific to security.
cy.waitForSearch(); | ||
}); | ||
|
||
describe('no line wrapping in legacy table', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no line wrapping in legacy table
--> auto line wrapping in legacy table
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the description.
}); | ||
|
||
describe('no line wrapping in legacy table', () => { | ||
it('checks that there is no line wrapping by default in the legacy table', function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here. Legacy table can auto do line wrapping with additional setting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the description.
// last element is _scrore if there is wrapping this field won't be present | ||
// So we check for the presence of the _score element in the legacy table | ||
|
||
cy.get('.euiDescriptionList__title').should('contain.text', '_score'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice test
}); | ||
}); | ||
|
||
describe('maxHeight With No truncation', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be cleaner if put maxHeight With No truncation
and maxHeight With truncation
together.
describe('maxHeight truncation behavior', () => {
it('checks if the table respects maxHeight setting with truncation', function () {
cy.setAdvancedSetting({
'truncate:maxHeight': 130,
});
cy.get('.truncate-by-height')
.first()
.should('have.css', 'max-height', '130px');
});
it('checks if the table respects maxHeight setting without truncation', function () {
cy.setAdvancedSetting({
'truncate:maxHeight': 0,
});
cy.get('.truncate-by-height')
.first()
.should('not.have.css', 'max-height', '130px');
});
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consolidated both the tests into one.
cy.get('[data-test-subj="dataGridRowCell"]') | ||
.first() | ||
.invoke('height') | ||
.should('be.gt', res); // Check if the new height is greater than previus height |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
smart test. love it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this test include all table related issues? Cuz I see you only mentioned on in the description about multi document but it seems to me that this test file has covered at least two issues. It is better to include all of them in the description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the tests related to the table will be housed within this file. This PR only covers the tests corresponding to opensearch-project/OpenSearch-Dashboards#6280. There will be follow up PR addressing other table related issues.
7c03294
to
ebb903d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems the new added test cases failed to pass, please take a look here.
b3f373f
to
86088bb
Compare
Signed-off-by: Suchit Sahoo <suchsah@amazon.com>
@SuZhou-Joe I have updated the tests to fix the failures. The newly added tests are now passing for both with and without security enabled. |
@LDrago27 Thanks for addressing all the failed cases. |
Signed-off-by: Suchit Sahoo <suchsah@amazon.com> (cherry picked from commit a964ed8)
Signed-off-by: Suchit Sahoo <suchsah@amazon.com>
Description
This adds functional tests for the issues described in opensearch-project/OpenSearch-Dashboards#6280.
Issues Resolved
opensearch-project/OpenSearch-Dashboards#5713
Issues Still Pending (Sub Issues within the Test Suite are mentioned within brackets)
The test suites are divided across two files discover_table and discover_advanced_settings. All the functional tests related to the Advanced Settings are grouped in discover_advanced_setting while the rest are part of discover_table settings.
Tests when run without security
discover_advanced_settings_no_security.spec.js.mp4
discover_table_no_security.spec.js.mp4
Tests when run with security
discover_advanced_settings_with_security.spec.js.mp4
discover_table_with_security.spec.js.mp4
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.