minichallenge4 solved #85

oscarfiend · 2021-08-12T07:00:49Z

ghost · 2021-08-13T16:17:34Z

package.json

+    "hooks": {}
+  },
+  "jest": {
+    "coveragePathIgnorePatterns": [


I think you should remove the next files for this config:

"/src/providers", "/src/providers", "/src/config", "/src/helpers/", "/src/utils/", "/src/index.js", "/src/components/Fortune", "/pages/NotFound", "/components/App/"

They are part of the current app, so they should be included on the code coverage testing.

Please remove and update the code coverage report.

Any question or comment let me know.

ghost · 2021-08-13T16:31:03Z

Acceptance Criteria

The search term is stored and retrieved from the Global Context correctly.
The appearance theme is stored on the Global Context and applied correctly to the App UI.
useReducer hook is implemented correctly to manage the Global State.

Bonus Points

Testing coverage is above 70%. (Please include a screenshot of the code coverage report in your PR description).

General comments

Make sure that you are adding the package-lock.json on git.
When I run the tests on my local machine, I have:

 TestingLibraryElementError: Unable to find an element by: [data-testid="/list_videos/i"]

    <body>
      <div>
        <h2>
          Error fetching data, try later
        </h2>
      </div>
    </body>

    <body>
      <div>
        <h2>
          Error fetching data, try later
        </h2>
      </div>
    </body>Error: Unable to find an element by: [data-testid="/list_videos/i"]

    <body>
      <div>
        <h2>
          Error fetching data, try later
        </h2>
      </div>
    </body>
Test Suites: 2 failed, 4 passed, 6 total
Tests:       3 failed, 21 passed, 24 total
Snapshots:   0 total
Time:        7.877s

I think is because the tests are fetching to the real youtube api.

When I run npm run lint, there are several errors, make sure to always run lint before of make a PR. You could use https://www.npmjs.com/package/husky

ghost · 2021-08-13T16:38:12Z

src/__tests__/videoDetails.test.js

+  });
+
+  test('should containt a video description', () => {
+    // const titleVideo = screen.findByText(/una librería para crear interfaces web/i);


remember to remove commented code

ghost · 2021-08-13T16:39:03Z

src/__tests__/videoList.test.js

+    const list = screen.findAllByTestId(/list_videos/i);
+    expect((await list).length).toBe(24)
+  });
+});


This are good test cases, but remember to mock the api call.https://jestjs.io/docs/mock-functions

ghost · 2021-08-13T16:53:45Z

Good idea to separate the React context, one for Theme and another one for Video api logic, good work :D

ghost · 2021-08-21T21:38:55Z

src/__tests__/header.test.jsx

+    const menu = screen.getByTestId(/hamburguerMenu/i);
+    expect(menu).toHaveAttribute('type', 'checkbox');
+  });
+});


Great test, I see that you are mocking the Api, good work!

ghost · 2021-08-21T21:40:19Z

src/__tests__/header.test.jsx

+
+  test('Toggle button should be checkbox type', () => {
+    const toggle = screen.getByRole('checkbox');
+    expect(toggle).toHaveAttribute('type', 'checkbox');


Since you are doing the query getByRole('checkbox'), there is explicitly that the element is a checkbox, so you could do the expect using toBeInTheDocument(), makes sense?

ghost · 2021-08-21T21:41:33Z

src/__tests__/home.test.jsx

+    const title = screen.getByText(/Youtube challenge/i);
+    expect(title).toBeInTheDocument();
+  });
+});


is this Home component actually fetching the API?

ghost · 2021-08-21T21:42:24Z

src/__tests__/videoCard.test.jsx

+    const videoCreation = screen.queryByText(video.publishedAt);
+    expect(videoCreation).toBeInTheDocument();
+  });
+});


Great work :D

ghost · 2021-08-21T21:45:01Z

src/__tests__/videoDetails.test.jsx

+  test('should containt a video iframe', () => {
+    waitFor(() => expect(screen.findByRole(/iframe/i)).toBeInTheDocument());
+  });
+});


Good test,

You can refactor this kind of expects:

waitFor(() => expect(screen.findByText(/Qué es React/i)).toBeInTheDocument());

to only

expect(await screen.findByText(/Qué es React/i)).toBeInTheDocument()

because findBy is a combination of getBy and waitFor: https://testing-library.com/docs/dom-testing-library/api-async/#findby-queries

ghost · 2021-08-21T21:47:52Z

src/config/axios.js

+  baseURL: 'https://content-youtube.googleapis.com/youtube/v3/',
+});
+
+export default axiosClient;


Good use of axios instance.

Go one more step ahead and try to put the baseURL value as Env variable, in order to get this more easy to change on the time.

ghost · 2021-08-21T21:54:32Z

src/context/video/videoReducer.js

+  UPDATE_QUERY_VIDEO,
+} from '../../utils/constants';
+
+export default (state = {}, { type, payload }) => {


Is a good idea to have the initial state with the default properties values

minichallenge4 solved with test coverage

0e0dce4

ghost reviewed Aug 13, 2021

View reviewed changes

oscarfiend added 2 commits August 18, 2021 21:55

mockapi was used in tests and lint ran

297103f

warnings in tests are fixed

074b3a3

ghost reviewed Aug 21, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

minichallenge4 solved #85

minichallenge4 solved #85

oscarfiend commented Aug 12, 2021

ghost Aug 13, 2021 •

edited by ghost

Loading

ghost commented Aug 13, 2021 •

edited by ghost

Loading

ghost Aug 13, 2021

ghost Aug 13, 2021

ghost commented Aug 13, 2021

ghost Aug 21, 2021

ghost Aug 21, 2021

ghost Aug 21, 2021

ghost Aug 21, 2021

ghost Aug 21, 2021

ghost Aug 21, 2021

ghost Aug 21, 2021

minichallenge4 solved #85

Are you sure you want to change the base?

minichallenge4 solved #85

Conversation

oscarfiend commented Aug 12, 2021

ghost Aug 13, 2021 • edited by ghost Loading

Choose a reason for hiding this comment

ghost commented Aug 13, 2021 • edited by ghost Loading

Acceptance Criteria

Bonus Points

General comments

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Aug 13, 2021

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

ghost Aug 13, 2021 •

edited by ghost

Loading

ghost commented Aug 13, 2021 •

edited by ghost

Loading