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

Adding tests for acceleration components #1495

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

sejli
Copy link
Member

@sejli sejli commented Mar 6, 2024

Description

Issues Resolved

N/A

Check List

  • New functionality includes testing.
    • All tests pass, including unit test, integration test and doctest
  • New functionality has been documented.
    • New functionality has javadoc added
    • New functionality has user manual doc added
  • Commits are signed per the DCO using --signoff

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.

Copy link

codecov bot commented Mar 6, 2024

Codecov Report

Attention: Patch coverage is 33.33333% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 55.09%. Comparing base (8874c8c) to head (56c851f).

❗ Current head 56c851f differs from pull request most recent head fe92fdc. Consider uploading reports for the commit fe92fdc to get more accurate results

Files Patch % Lines
...onents/manage/accelerations/acceleration_table.tsx 33.33% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1495      +/-   ##
==========================================
- Coverage   56.98%   55.09%   -1.90%     
==========================================
  Files         348      365      +17     
  Lines       12705    13134     +429     
  Branches     3214     3303      +89     
==========================================
- Hits         7240     7236       -4     
- Misses       5412     5843     +431     
- Partials       53       55       +2     
Flag Coverage Δ
dashboards-observability 55.09% <33.33%> (-1.90%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

it('Render empty acceleration table', () => {
const wrapper = mount(<AccelerationTable accelerations={[]} />);

expect(wrapper).toMatchSnapshot();
Copy link
Member

@ps48 ps48 Mar 7, 2024

Choose a reason for hiding this comment

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

Let's add more interaction tests for flyouts and table once we connect these with the backend. These should be good for now

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1, you can mock like this:

jest.mock('../../../../plugin', () => ({
  getRenderAccelerationDetailsFlyout: jest.fn(() =>
    jest.fn().mockImplementation(() => console.log('Acceleration Details Flyout Rendered'))
  ),
  getRenderAssociatedObjectsDetailsFlyout: jest.fn(() =>
    jest.fn().mockImplementation(() => console.log('Associated Objects Details Flyout Rendered'))
  ),
}));

so that we can test the render:

 it('renders acceleration details correctly and triggers flyout on click', () => {
   const wrapper = mount(<AssociatedObjectsDetailsFlyout tableDetail={mockTableDetail} />);
   expect(wrapper.find('EuiInMemoryTable').at(0).find('EuiLink').length).toBeGreaterThan(0);

   wrapper.find('EuiInMemoryTable').at(0).find('EuiLink').first().simulate('click');
   expect(plugin.getRenderAccelerationDetailsFlyout).toHaveBeenCalled();
 });

Reference: https://github.com/opensearch-project/dashboards-observability/pull/1496/files#diff-9a67ab280ec2234577feebc0875415a357dcf4e0f6f4d264a96b18ea5264221eR32

configure({ adapter: new Adapter() });

afterEach(() => {
cleanup();
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is this clean up for?

it('Render empty acceleration table', () => {
const wrapper = mount(<AccelerationTable accelerations={[]} />);

expect(wrapper).toMatchSnapshot();
Copy link
Collaborator

Choose a reason for hiding this comment

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

+1, you can mock like this:

jest.mock('../../../../plugin', () => ({
  getRenderAccelerationDetailsFlyout: jest.fn(() =>
    jest.fn().mockImplementation(() => console.log('Acceleration Details Flyout Rendered'))
  ),
  getRenderAssociatedObjectsDetailsFlyout: jest.fn(() =>
    jest.fn().mockImplementation(() => console.log('Associated Objects Details Flyout Rendered'))
  ),
}));

so that we can test the render:

 it('renders acceleration details correctly and triggers flyout on click', () => {
   const wrapper = mount(<AssociatedObjectsDetailsFlyout tableDetail={mockTableDetail} />);
   expect(wrapper.find('EuiInMemoryTable').at(0).find('EuiLink').length).toBeGreaterThan(0);

   wrapper.find('EuiInMemoryTable').at(0).find('EuiLink').first().simulate('click');
   expect(plugin.getRenderAccelerationDetailsFlyout).toHaveBeenCalled();
 });

Reference: https://github.com/opensearch-project/dashboards-observability/pull/1496/files#diff-9a67ab280ec2234577feebc0875415a357dcf4e0f6f4d264a96b18ea5264221eR32

@@ -122,7 +132,12 @@ export const AccelerationDetailsFlyout = (props: AccelerationDetailsFlyoutProps)
</EuiFlexItem>
</EuiFlexGroup>
<EuiSpacer size="m" />
<EuiTabs style={{ marginBottom: '-25px' }}>{renderTabs()}</EuiTabs>
<EuiTabs
style={{ marginBottom: '-25px' }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should avoid this inline style, and we can switch to a SCSS to define this. But yeah, this is not related to this PR, and we can do that as a follow up.

RyanL1997 pushed a commit to RyanL1997/dashboards-observability that referenced this pull request Apr 18, 2024
…tion menu (opensearch-project#1474) (opensearch-project#1495)

* move security management section

Signed-off-by: Hailong Cui <[email protected]>

* Fix eslint

Signed-off-by: Hailong Cui <[email protected]>

* Remove plugins pages for management overview registration

Signed-off-by: Hailong Cui <[email protected]>

---------

Signed-off-by: Hailong Cui <[email protected]>
Co-authored-by: Darshit Chanpura <[email protected]>
Co-authored-by: Craig Perkins <[email protected]>
(cherry picked from commit c74973af03efa8b1139864b3779c63450a64e585)

Co-authored-by: Hailong Cui <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants