Migrate App/Page.test.js from Enzyme to RTL

[anshgoyalevil]

Which problem is this PR solving?

• This PR migrates the App/Page.test.js from Enzyme's render function to RTL's render function

Description of the changes

• It uses the official migration guide as a reference to migration : https://testing-library.com/docs/react-testing-library/migrate-from-enzyme/

How was this change tested?

• By running the required tests

Checklist

• I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
• I have signed all commits
• I have added unit tests for the new functionality

[codecov]

Codecov Report

Patch and project coverage have no change.

Comparison is base (9eb0d7a) 96.01% compared to head (9ba883f) 96.01%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1659   +/-   ##
=======================================
  Coverage   96.01%   96.01%           
=======================================
  Files         241      241           
  Lines        7558     7558           
  Branches     1984     1984           
=======================================
  Hits         7257     7257           
  Misses        301      301           

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

[anshgoyalevil]

@yurishkuro I tried out the migration from Enzyme to RTL, and it just works. Could you please review it.

[yurishkuro]

why not replace this with render? This was the initial assumption in the test, and you're changing it significantly

[anshgoyalevil]

We can't directly replace the mount from enzyme with render from RTL.
It would include a significant amount of code changes.

What type of changes, to be precise?

• In React Testing Library, we don't need to assign the render result to a variable (i.e. wrapper).
• Even if we assign the render result to a wrapper, it doesn't have functions like toBeDefined, find, etc.
• There are some other new functions introduced like getByRole, toHaveTextContent etc in the RTL.

These changes require a lot of code changes, which are not only limited to test files, but may also need changes to the components, like giving them test id, or ARIA role, so they can be selected by RTL while writing the tests.

To understand the migration land space more clearly, there's an official Sentry's blog on this topic, according to which, it took them around an year to migrate all tests (around 500+), while modifying 8 per week.

Numbers would be different for everyone, but this gives us an idea that it would require significant code changes to be completely shifted to RTL.

[anshgoyalevil]

Talking about the current test (Page.test.js), the wrapper is defined and contains the page elements, so it was initialized inside the describe block so that the test cases can use it.

For example,
in the first test case,

it('does not explode', () => {
    expect(wrapper).toBeDefined();
});

We are checking for the wrapper to be defined, but in case of RTL, there's nothing like a wrapper at all.
So, to check if it doesn't explode, we need to render it inside the test case.

Similarly, other test cases needed modifications to fit according to RTL.

[anshgoyalevil]

How to check if the modifications are correct?

• Introduce false errors
  We can introduce false errors to the expect statements to check if they only give positive results with correct arguments.

For example:

it('tracks an initial page-view', () => {
    const { pathname, search } = props;
    render(<Page {...props} />);
    expect(trackPageView).toHaveBeenCalledWith(pathname, search);
});

In this test case, the value of pathname and search is a random value casted to a string.

Making it something like:

it('tracks an initial page-view', () => {
    const { pathname, search } = props;
    render(<Page {...props} />);
    expect(trackPageView).toHaveBeenCalledWith("ABC", search);
});

gives the following error:

● <Page> › tracks an initial page-view

    expect(jest.fn()).toHaveBeenCalledWith(...expected)

    Expected: "ABC", "0.06260740645434315"
    Received: "0.8310162119478424", "0.06260740645434315"

    Number of calls: 1

      52 |     const { pathname, search } = props;
      53 |     render(<Page {...props} />);
    > 54 |     expect(trackPageView).toHaveBeenCalledWith("ABC", search);
         |                           ^
      55 |   });
      56 |
      57 |   it('tracks a pageView when the location changes', () => {

      at Object.toHaveBeenCalledWith (src/components/App/Page.test.js:54:27)

Which clearly states that tests are running as expected.

[yurishkuro]

I disagree with your reasoning.

• You're calling render in the test anyway, so surely it can be called in beforeTest if this is how the tests were written
• RTL has screen that is the equivalent of the returned wrapper
• 'does not explode' test can be modified to look for some specific element in the screen object

[anshgoyalevil]

I was using the render function in each of those test cases, to make sure I don't have to rely on the jest-dom dependency, which provides the functions for screen object.

I have updated the test code to use the screen object.

A quick question:
Since, if we go with the screen approach, the jest-dom dep needs to be imported in each test file that needs it.
In jest, we can globalise imports using the setupTests.js file. Shall I create one?

[yurishkuro]

thanks!