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.

Sign up for GitHub

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

Jump to bottom

packages/dev-console/src/component/project-access: Add tests #3820

Merged
merged 1 commit into from Jan 12, 2020
Merged

packages/dev-console/src/component/project-access: Add tests #3820

merged 1 commit into from Jan 12, 2020

Conversation

debsmita1
Copy link
Contributor

Added tests for :

  • ProjectAccess
  • ProjectAccessPage
  • RenderProjectAccessPage Components

Refer Jira tasks :

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. component/dev-console Related to dev-console labels Dec 20, 2019
christianvogt
christianvogt reviewed Dec 21, 2019
View reviewed changes
frontend/packages/dev-console/src/components/project-access/RenderProjectAccessPage.tsx Outdated
@@ -40,4 +42,4 @@ const RenderProjectAccessPage: React.FC<RenderProjectAccessPageProps> = ({ names
);
};

export default withStartGuide(RenderProjectAccessPage);
export const RenderProjectAccessPage = withStartGuide(RenderProjectAccessPageComponent);
Copy link
Contributor

Choose a reason for hiding this comment

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

Change back to a default export.

frontend/packages/dev-console/src/components/project-access/__tests__/ProjectAccess.spec.tsx Outdated
import { LoadingBox, StatusBox } from '@console/internal/components/utils';
import ProjectAccess, { ProjectAccessProps } from '../ProjectAccess';

const projectAccessProps: ProjectAccessProps = {
Copy link
Contributor

Choose a reason for hiding this comment

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

We are now using React.ComponentProps instead of exporting the props type.
Please update the references.

You are also mutating the same instance across multiple tests. You should be creating it new before each test.

frontend/packages/dev-console/src/components/project-access/RenderProjectAccessPage.tsx Outdated
@@ -8,7 +8,9 @@ export interface RenderProjectAccessPageProps {
namespace: string;
}

const RenderProjectAccessPage: React.FC<RenderProjectAccessPageProps> = ({ namespace }) => {
export const RenderProjectAccessPageComponent: React.FC<RenderProjectAccessPageProps> = ({
Copy link
Contributor

Choose a reason for hiding this comment

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

Change name back to RenderProjectAccessPage once the default export is reverted.

frontend/packages/dev-console/src/components/project-access/__tests__/ProjectAccess.spec.tsx Outdated
};

describe('Project Access', () => {
let renderProjectAccess: ShallowWrapper<ProjectAccessProps>;
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for the declaration to be here. Can leave it inside the individual tests.

frontend/packages/dev-console/src/components/project-access/__tests__/ProjectAccess.spec.tsx Outdated
it('should show the LoadingBox when role bindings are not loaded, but user has access to role bindings', () => {
projectAccessProps.roleBindings.loaded = false;
renderProjectAccess = shallow(<ProjectAccess {...projectAccessProps} />);
expect(renderProjectAccess.exists()).toBe(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

This assertion is unnecessary because the wrapper will always exist.
enzymejs/enzyme#1864

...tend/packages/dev-console/src/components/project-access/__tests__/ProjectAccessPage.spec.tsx Outdated

describe('Project Access Page', () => {
let wrapper: ShallowWrapper<ProjectAccessPageProps>;
let projectAccessPageProps;
Copy link
Contributor

Choose a reason for hiding this comment

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

Unnecessary to have declaration of props here.

abhinandan13jan
abhinandan13jan reviewed Dec 24, 2019
View reviewed changes
frontend/packages/dev-console/src/components/project-access/ProjectAccessPage.tsx
@@ -1,7 +1,7 @@
import * as React from 'react';
import { match as RMatch } from 'react-router';
import { NamespaceBar } from '@console/internal/components/namespace';
import RenderProjectAccessPage, { RenderProjectAccessPageProps } from './RenderProjectAccessPage';
import RenderProjectAccess from './RenderProjectAccessPage';
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the name change is required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Otherwise, it gives import/no-named-as-default lint error. The RenderProjectAccess points to the default export in the RenderProjectAccessPage.tsx file. Please do let me know if there's any other alternative for this

Copy link
Contributor

Choose a reason for hiding this comment

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

What I meant is,
why not
import RenderProjectAccessPage from './RenderProjectAccessPage'
in lieu of import RenderProjectAccess from './RenderProjectAccessPage'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the RenderProjectAccessPage component, I have used RenderProjectAccessPage as default export (for real usage) and named export (for testing). Because of this change it gives me an import/no-named-as-default lint error in the ProjectAccessPage.tsx file where the component is being imported. To fix this error I had to import it like import RenderProjectAccess from './RenderProjectAccessPage', here RenderProjectAccess points to the default export

@abhinandan13jan
Copy link
Contributor

squash commits into 1

@debsmita1 debsmita1 force-pushed the project-access-tests branch 2 times, most recently from 840526a to 5a11439 Compare December 24, 2019 14:11
sahil143
sahil143 suggested changes Dec 27, 2019
View reviewed changes
frontend/packages/dev-console/src/components/project-access/__tests__/ProjectAccess.spec.tsx Outdated

describe('Project Access', () => {
it('should show the LoadingBox when role bindings are not loaded, but user has access to role bindings', () => {
const projectAccessProps: ProjectAccessProps = {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can declare this inside beforeEach block and change the specific prop of this object in each test case. No need to declare this in each test case

@debsmita1
add tests for Project Access
b410843 
move test data from `mocks` to `tests` folder
@andrewballantyne
Copy link
Contributor

/kind cleanup

@openshift-ci-robot openshift-ci-robot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Jan 2, 2020
andrewballantyne
andrewballantyne approved these changes Jan 6, 2020
View reviewed changes
Copy link
Contributor

@andrewballantyne andrewballantyne left a comment

Choose a reason for hiding this comment

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

/assign

frontend/packages/dev-console/src/components/project-access/RenderProjectAccessPage.tsx
Comment on lines +12 to 15
const props: React.ComponentProps<typeof ProjectAccess> = {
formName: 'project access',
namespace,
};
Copy link
Contributor

Choose a reason for hiding this comment

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

This variable creation is a bit bizarre in itself... was there any reason why you separated this into a variable instead of directly on the component? (originally when you did the feature - no change needed now, just curious)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@andrewballantyne yes, I could have directly passed formName and namespace. It might have gone unnoticed when I did the feature

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 6, 2020
@debsmita1
Copy link
Contributor Author

/test e2e-gcp-console

1 similar comment
@andrewballantyne
Copy link
Contributor

/test e2e-gcp-console

@invincibleJai
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 10, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, debsmita1, invincibleJai

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@spadgett
Copy link
Member

/hold
due to a bug in CI

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 10, 2020
@spadgett
Copy link
Member

/test e2e-gcp-console

@spadgett
Copy link
Member

/test analyze

@spadgett
Copy link
Member

/hold cancel
/retest

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 11, 2020
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

9 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit dacb3cd into openshift:master Jan 12, 2020
@spadgett spadgett added this to the v4.4 milestone Jan 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@christianvogt christianvogt christianvogt left review comments

@abhinandan13jan abhinandan13jan abhinandan13jan left review comments

@andrewballantyne andrewballantyne andrewballantyne approved these changes

@abhi-kn abhi-kn Awaiting requested review from abhi-kn

@vikram-raj vikram-raj Awaiting requested review from vikram-raj

@sahil143 sahil143 Awaiting requested review from sahil143

Assignees

@invincibleJai invincibleJai

@andrewballantyne andrewballantyne

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/dev-console Related to dev-console kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v4.4
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@debsmita1 @abhinandan13jan @andrewballantyne @invincibleJai @openshift-ci-robot @spadgett @openshift-bot @sahil143 @christianvogt @openshift-merge-robot