test(support-tools-page): add testing for support tools page #70

awaisdar001 · 2020-10-29T10:24:26Z

Add Config for testing for the support tools app.

Adding Unit tests for Users app
Adding API error tests
Moving isEmail, isValidUsername to a common utils file.
Addressed Eslint errors
Move messages to messages.js (Only a few messages have been moved which are used in tests, in future, all can be moved error messages can be moved here).

Adds the following npm commands.

npm run debug-test => for running tests in debug mode.
npm run snapshot => updates the snapshots

Coverage from 0% => ~23%

https://openedx.atlassian.net/browse/PROD-2178

codecov · 2020-10-29T10:27:06Z

Codecov Report

❗ No coverage uploaded for pull request base (master@5f485f8). Click here to learn what that means.
The diff coverage is 75.86%.

@@            Coverage Diff            @@
##             master      #70   +/-   ##
=========================================
  Coverage          ?   21.86%           
=========================================
  Files             ?       28           
  Lines             ?      709           
  Branches          ?      152           
=========================================
  Hits              ?      155           
  Misses            ?      539           
  Partials          ?       15

Impacted Files	Coverage Δ
src/dates/formatDate.js	`100.00% <ø> (ø)`
src/index.jsx	`0.00% <ø> (ø)`
src/users/CourseSummary.jsx	`12.50% <0.00%> (ø)`
src/users/EnrollmentForm.jsx	`12.50% <ø> (ø)`
src/users/EntitlementForm.jsx	`14.81% <ø> (ø)`
src/users/Entitlements.jsx	`2.94% <ø> (ø)`
src/users/UserSummary.jsx	`3.44% <ø> (ø)`
src/users/data/api.js	`26.80% <55.55%> (ø)`
src/components/common/PageLoading.jsx	`100.00% <100.00%> (ø)`
src/support-home/SupportHomePage.jsx	`100.00% <100.00%> (ø)`
... and 31 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5f485f8...024293e. Read the comment docs.

DawoudSheraz · 2020-11-02T14:04:53Z

src/components/__snapshots__/SupportLinksPage.test.jsx.snap

+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`SupportLinksPage matches snapshot 1`] = `
+<main
+  className="container-fluid m-5"
+>
+  <h3>
+    Support Tools
+  </h3>
+  <ul>
+    <li>
+      <a
+        href="/users"
+        id="search-users"
+        onClick={[Function]}
+      >
+        Search Users
+      </a>
+    </li>
+  </ul>
+</main>
+`;


I am not sure if this file should be committed. Is there a specific reason for this file?

yes, this is the snapshot file which would be required for comparing current (at the time of running builds) and expected snapshot (this one)

DawoudSheraz · 2020-11-02T14:09:21Z

src/components/SupportLinksPage.test.jsx

+  it('matches snapshot', () => {
+    const tree = renderer.create(<SupportLinksPage />).toJSON();
+    expect(tree).toMatchSnapshot();


What does this achieve?

toMatchSnapshot(); this matches the current snapshot with expected (saved) snapshot.

DawoudSheraz

For future reference, ideally, the component where the UI changes are more important should be tested via snapshots. For the simple components, the regular unit tests will be enough.

DawoudSheraz · 2020-11-04T09:49:26Z

src/PageLoading.test.jsx

+  it('does not render without message', () => {
+    const wrapper = shallow(<PageLoading srMessage="" />);
+
+    expect(wrapper.find('.sr-only')).toHaveLength(0);
+  });
+  it('does not render without message', () => {


both have the same description. 2nd should be renders correctly with message

DawoudSheraz · 2020-11-04T09:49:54Z

src/PageLoading.test.jsx

+    expect(srElement).toHaveLength(1);
+    expect(srElement.text()).toEqual(message);
+  });
+  it('renders correctly', () => {


--> snapshot matches correctly

DawoudSheraz · 2020-11-04T09:53:48Z

src/components/users/SupportLinksPage.test.jsx

+
+    expect(searchUsersText).toEqual('Search Users');
+  });
+  it('have correct heading', () => {


DawoudSheraz · 2020-11-04T09:53:56Z

src/components/users/SupportLinksPage.test.jsx

+  it('correctly renders', () => {
+    expect(wrapper.find('li')).toHaveLength(1);
+  });
+  it('have search users link', () => {


DawoudSheraz · 2020-11-04T09:55:40Z

src/users/UserPage.test.jsx

+  describe('Checking PropTypes', () => {
+    it('Should not throw a warning', () => {
+      const expectedProps = { location };
+      const propsError = checkProps(UserPage, expectedProps);
+
+      expect(propsError).toBeUndefined();
+    });
+  });


Why is there a separate describe for a single test?

DawoudSheraz · 2020-11-04T09:55:49Z

src/users/UserPage.test.jsx

+    });
+  });
+  //
+  it('Renders Correctly', () => {


snapshot match

just to organize tests. I have added a separate test for proptypes tests.

DawoudSheraz · 2020-11-04T09:56:28Z

src/users/UserSearch.test.jsx

+      );
+    });
+
+    it('using snapshot', () => {


matches snapshot

DawoudSheraz

A few comments/questions to address

DawoudSheraz · 2020-11-05T13:31:30Z

src/dates/formatDate.test.js

+
+    expect(formatedDate).toEqual('Sep 19, 2015 11:00 AM');
+  });
+  it('fallback to N/A if no args are passed', () => {


nit: returns N/A if data is not provided

DawoudSheraz · 2020-11-05T13:33:23Z

src/user-messages/messages.js

+export const USER_IDENTIFIER_INVALID_ERROR = 'The searched username or email is invalid. Please correct the username or email and try again.';
+export const USERNAME_IDENTIFIER_NOT_FOUND_ERROR = 'We couldn\'t find a user with the username "{identifier}".';
+export const USER_EMAIL_IDENTIFIER_NOT_FOUND_ERROR = 'We couldn\'t find a user with the email "{identifier}".';
+export const UNKNOWN_API_ERROR = "There was an error loading this user's data. Check the JavaScript console for detailed errors.";


I don't know the current behavior but is there i18n setup for such texts?

I am not aware of that, and these were not being translated currently. I am also not sure if there are plans adding translations for those strings.

DawoudSheraz · 2020-11-05T13:35:12Z

src/users/UserPage.test.jsx

+// Function to wait until the entire component is fully painted.
+const waitForComponentToPaint = async (wrapper) => {
+  await act(async () => {
+    await new Promise((resolve) => setTimeout(resolve));
+    wrapper.update();
+  });
+};


What does component paint stands for?

Ah, it just means that component is fully rendered after all promise based function calls.

DawoudSheraz · 2020-11-05T13:37:51Z

src/users/UserPage.test.jsx

+    });
+  });
+  describe('shows expected error alert', () => {
+    const mockAuthApiToThrowError = (code) => {


mockAuthResponseError or something similar to this.

DawoudSheraz · 2020-11-05T13:39:35Z

src/users/UserPage.test.jsx

+    it('when user identifier is not found', async () => {
+      const validUsername = 'valid-non-existing-username';
+      location.search = `?username=${validUsername}`;
+      mockAuthApiToThrowError(404);
+      const expectedAlert = messages.USERNAME_IDENTIFIER_NOT_FOUND_ERROR.replace(
+        '{identifier}',
+        validUsername,
+      );
+
+      const wrapper = mount(<UserPageWrapper location={location} />);
+      await waitForComponentToPaint(wrapper);
+
+      const alert = wrapper.find('.alert');
+      expect(alert).toHaveLength(1);
+      expect(alert.text()).toEqual(expectedAlert);
+    });
+    it('when user email is not found', async () => {


I haven't seen async usage inside tests before. Can you share an existing example from edX codebase?

Here is one example.
https://github.com/edx/frontend-app-learning/blob/927d424d339a8ecde9c542b446770a82bc7dc3cd/src/instructor-toolbar/InstructorToolbar.test.jsx#L47-L54

DawoudSheraz · 2020-11-05T13:40:36Z

src/users/UserSearch.test.jsx

+
+      expect(propsError).toBeUndefined();
+    });
+    it('does throw error with undefined search handler', () => {


throws error. not need for does in the beginning.

DawoudSheraz · 2020-11-05T13:46:09Z

src/utils/index.js

+const EMAIL_REGEX = '^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\\.[a-zA-Z]{2,}$';
+const USERNAME_REGEX = '^[\\w.@_+-]+$';
+
+export const isEmail = (value) => !!(value && value.match(EMAIL_REGEX));


No need for !!. This operation is not affecting the value.

@DawoudSheraz I have copied the methods as is, we can plan on refactoring in the other ticket. I can add todo here as well.

DawoudSheraz · 2020-11-05T13:46:20Z

src/utils/index.js

+
+/* eslint-disable arrow-body-style */
+export const isValidUsername = (searchValue) => {
+  return !!(searchValue && searchValue.match(USERNAME_REGEX));


awaisdar001 · 2020-11-06T11:34:12Z

@DawoudSheraz updated the PR.

saadyousafarbi

looks good.

asadazam93 · 2020-11-09T12:44:26Z

src/dates/formatDate.test.js

+import formatDate from './formatDate';
+
+describe('Format Date', () => {
+  it('correctly formats date', () => {


Do we need to test the utilities as well? I think the priority should be the React components for now

Yes, true, I did this to bump to coverage a little bit :) and they were easy enough

asadazam93

Looks good to me

awaisdar001 requested review from asadazam93 and DawoudSheraz November 2, 2020 07:18

DawoudSheraz reviewed Nov 2, 2020

View reviewed changes

awaisdar001 force-pushed the add-testing-for-support-tools branch 2 times, most recently from b8d4759 to 6025560 Compare November 3, 2020 11:03

DawoudSheraz approved these changes Nov 4, 2020

View reviewed changes

awaisdar001 force-pushed the add-testing-for-support-tools branch 3 times, most recently from 2a8f4ce to 6f244b6 Compare November 5, 2020 05:36

awaisdar001 requested a review from DawoudSheraz November 5, 2020 07:04

DawoudSheraz reviewed Nov 5, 2020

View reviewed changes

awaisdar001 force-pushed the add-testing-for-support-tools branch from 33fc72d to 4fbb9fd Compare November 6, 2020 11:34

test(support-tools-page): add testing for support tools page

024293e

awaisdar001 force-pushed the add-testing-for-support-tools branch from 01239de to 024293e Compare November 9, 2020 09:39

saadyousafarbi approved these changes Nov 9, 2020

View reviewed changes

asadazam93 reviewed Nov 9, 2020

View reviewed changes

asadazam93 approved these changes Nov 9, 2020

View reviewed changes

awaisdar001 merged commit a5971d3 into master Nov 9, 2020

awaisdar001 deleted the add-testing-for-support-tools branch November 9, 2020 13:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(support-tools-page): add testing for support tools page #70

test(support-tools-page): add testing for support tools page #70

awaisdar001 commented Oct 29, 2020 •

edited

Loading

codecov bot commented Oct 29, 2020 •

edited

Loading

DawoudSheraz Nov 2, 2020

awaisdar001 Nov 3, 2020

DawoudSheraz Nov 2, 2020

awaisdar001 Nov 3, 2020

DawoudSheraz left a comment

DawoudSheraz Nov 4, 2020

DawoudSheraz Nov 4, 2020

DawoudSheraz Nov 4, 2020

DawoudSheraz Nov 4, 2020

DawoudSheraz Nov 4, 2020

DawoudSheraz Nov 4, 2020

awaisdar001 Nov 4, 2020

DawoudSheraz Nov 4, 2020

DawoudSheraz left a comment

DawoudSheraz Nov 5, 2020

DawoudSheraz Nov 5, 2020

awaisdar001 Nov 5, 2020

DawoudSheraz Nov 5, 2020

awaisdar001 Nov 5, 2020

DawoudSheraz Nov 5, 2020

DawoudSheraz Nov 5, 2020

awaisdar001 Nov 6, 2020

DawoudSheraz Nov 5, 2020

DawoudSheraz Nov 5, 2020

awaisdar001 Nov 6, 2020

DawoudSheraz Nov 5, 2020

awaisdar001 commented Nov 6, 2020

saadyousafarbi left a comment

asadazam93 Nov 9, 2020

awaisdar001 Nov 9, 2020

asadazam93 left a comment

test(support-tools-page): add testing for support tools page #70

test(support-tools-page): add testing for support tools page #70

Conversation

awaisdar001 commented Oct 29, 2020 • edited Loading

Add Config for testing for the support tools app.

codecov bot commented Oct 29, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DawoudSheraz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DawoudSheraz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awaisdar001 commented Nov 6, 2020

saadyousafarbi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asadazam93 left a comment

Choose a reason for hiding this comment

awaisdar001 commented Oct 29, 2020 •

edited

Loading

codecov bot commented Oct 29, 2020 •

edited

Loading