fix: enzyme testing replaced by react tester #36159

jciasenza · 2025-01-23T13:43:52Z

Replaced enzyme display dropdown on toggling dropdown test in Main file for react-renderer tests.

I made PR of this issue #35245

And Testing OK

If something needs to be modified, let me know, thank you!!!
Atte
Juan Carlos (Aulasneo)

Developer Checklist

Test suites passing
Documentation and test plan updated, if applicable
Received code-owner approving review

openedx-webhooks · 2025-01-23T13:43:57Z

Thanks for the pull request, @jciasenza!

This repository is currently maintained by @openedx/wg-maintenance-edx-platform.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

jciasenza · 2025-01-23T14:07:32Z

I don't understand, locally the changes pass all the tests and those that appear as failures are not files that have been modified.

robrap · 2025-01-23T15:35:07Z

I'm re-running the failed tests. I think you just hit a flaky test failure.
You may still need to look into the quality failure.

jciasenza · 2025-01-23T15:40:34Z

for example in this case, quality checks/Quality Others (ubuntu-24.04, 3.11, 20) (pull_request), I have not changed anything

robrap · 2025-01-23T16:18:26Z

You need to click the "Details" link to see what the actual failures are. There are ESlint failures.

Error: /home/runner/work/edx-platform/edx-platform/lms/djangoapps/instructor/static/instructor/ProblemBrowser/components/Main/Main.test.jsx: line 34, col 24, Error - Missing trailing comma. (comma-dangle)
Error: /home/runner/work/edx-platform/edx-platform/lms/djangoapps/instructor/static/instructor/ProblemBrowser/components/Main/Main.test.jsx: line 58, col 24, Error - Missing trailing comma. (comma-dangle)
Error: /home/runner/work/edx-platform/edx-platform/lms/djangoapps/instructor/static/instructor/ProblemBrowser/components/Main/Main.test.jsx: line 64, col 45, Error - Block must not be padded by blank lines. (padded-blocks)
Error: /home/runner/work/edx-platform/edx-platform/lms/djangoapps/instructor/static/instructor/ProblemBrowser/components/Main/Main.test.jsx: line 83, col 24, Error - Missing trailing comma. (comma-dangle)

jciasenza · 2025-01-23T17:45:05Z

@robrap Now they passed the tests!!! Thaks!!!

robrap · 2025-01-23T18:37:45Z

[inform] I create the following ticket to fix the flaky test failure issue:

Upload-artifact error in xmodule-with-lms/xmodule-with-cms tests in CI #36160

package.json

jciasenza · 2025-01-28T20:27:00Z

Hello @brian-smith-tcril, when you can check, I think this PR would be resolved now, Thanks !

brian-smith-tcril

Overall this looks great! Super happy to see things moving to RTL!

One thing I noticed is that the new tests that are replacing the snapshot tests aren't verifying the "Create a report of problem responses" button exists.

I also left a couple comments in the tests themselves. Nothing big, but definitely a couple spots where the tests could be more robust.

Thank you so much for doing this!

lms/djangoapps/instructor/static/instructor/ProblemBrowser/components/Main/Main.test.jsx

brian-smith-tcril · 2025-01-31T20:29:51Z

lms/djangoapps/instructor/static/instructor/ProblemBrowser/components/Main/Main.test.jsx

+        await userEvent.click(toggleButton);
+
+        await userEvent.click(toggleButton);


I think an extra expect in between these to verify the dropdown actually opens and closes (as opposed to just not ever opening) would probably be good.

Thanks @brian-smith-tcril for giving me the feedback, I will review those points and upload the changes again.

…tform into jci/issue35245

jciasenza · 2025-02-03T15:25:14Z

Hello, @brian-smith-tcril,
I made more changes to see what you think how they work now.

brian-smith-tcril · 2025-02-03T16:07:11Z

lms/djangoapps/instructor/static/instructor/ProblemBrowser/components/Main/Main.test.jsx

+        expect(screen.queryByText('Some expected block name')).toBeNull();
+        const toggleButton = screen.getByRole('button', { name: 'Select a section or problem' });
+        await userEvent.click(toggleButton);
+        const blockName = screen.queryByText('Some expected block name');
+        expect(blockName).toBeNull();


This seems a bit odd to me, it's expecting the query for 'Some expected block name' to be null both before and after clicking the toggle button?

I don't know if the way I modified it is right, but the tests passed.

brian-smith-tcril · 2025-02-03T17:54:22Z

lms/djangoapps/instructor/static/instructor/ProblemBrowser/components/Main/Main.test.jsx

+        const toggleButton = screen.getByRole('button', { name: 'Select a section or problem' });
+        await userEvent.click(toggleButton);
+        await waitFor(() => {
+            expect(screen.queryByText('Select a section or problem')).not.toBeNull();
+        });
+        await userEvent.click(toggleButton);
+        await waitFor(() => {
+            expect(screen.queryByText('Some expected block name')).toBeNull();
+        });


This looks great now!

Thaks you !!!

Actually sorry it doesn't. I missed the fact that you're now checking for 'Select a section or problem' to not be null, it should be 'Some expected block name' that is not null when toggling the dropdown

I had my doubts about what you told me but it gives me errors like it doesn't find "Some expected block name", but hey it's progress

in this case
expect(screen.queryByText('Select a section or problem')).not.toBeNull();

This is where I get the error arrives null or not found'Some expected block name'

ProblemBrowser Main component › hide dropdown on second toggle expect(received).not.toBeNull() Received: null

can you think of a solution?

After looking at this more my main question is: where is 'Some expected block name' coming from? I don't see anything in the tests putting that string anywhere.

The previous implementation of this test just looked for BlockBrowserContainer, it'd be helpful to know what is actually being rendered in that.

I don't know if it is a good solution but replace the texts with data-testid="block-browser-container"

brian-smith-tcril · 2025-02-04T18:26:56Z

I think part of what is making this PR hard for me to review is a lack of understanding of what is actually being tested. If you could take some screenshots of this running in a browser that would provide a lot of missing context for me.

jciasenza · 2025-02-04T19:20:04Z

Ok, it's really the enzyme test that is being replaced by the reaction tester.

I think the tests are replaced and pass well.

brian-smith-tcril · 2025-02-04T22:11:30Z

I don't see the dropdown in that screenshot.

it's really the enzyme test that is being replaced by the reaction tester.

While that is true, the old tests relied on snapshots, where the new tests are manually validating the behavior. The part of this I'm not understanding is what the tests should be looking for in the dropdown when it is toggled. It's not clear to me why an element with the test id block-browser-containeris what we want to test for.

jciasenza · 2025-02-05T01:26:57Z

I was reading a little and I think this is a good summary of the tests:

brian-smith-tcril

I have gone through and documented the issues I see with each of the tests in here. If you have questions about the issues I found please let me know.

brian-smith-tcril · 2025-02-05T15:50:03Z

lms/djangoapps/instructor/static/instructor/ProblemBrowser/components/Main/Main.test.jsx

                />
            </Provider>,
        );
-        const tree = component.toJSON();
-        expect(tree).toMatchSnapshot();
+        expect(screen.getByRole('button', { name: 'Select a section or problem' })).toBeInTheDocument();
    });


This test used to use a snapshot. That snapshot resulted in the following output

exports[`ProblemBrowser Main component render with basic parameters 1`] = ` <div className="problem-browser-container" > <div className="problem-browser" > <button className="btn" onBlur={[Function]} onClick={[Function]} onKeyDown={[Function]} type="button" > Select a section or problem </button> <input disabled={true} hidden={false} name="problem-location" type="text" value={null} /> <button className="btn" name="list-problem-responses-csv" onBlur={[Function]} onClick={[Function]} onKeyDown={[Function]} type="button" > Create a report of problem responses </button> <div aria-live="polite" className="report-generation-status" /> </div> </div> `;

This test is now only checking for the 'Select a section or problem' button. That is a large change in behavior from before.

To merge this PR this test needs to be updated to test everything it used to be testing.

brian-smith-tcril · 2025-02-05T15:52:08Z

lms/djangoapps/instructor/static/instructor/ProblemBrowser/components/Main/Main.test.jsx

                />
            </Provider>,
        );
-        const tree = component.toJSON();
-        expect(tree).toMatchSnapshot();
+        expect(screen.getByRole('button', { name: 'Select a section or problem' })).toBeInTheDocument();
    });


This test also used to use a snapshot. That snapshot resulted in the following output

exports[`ProblemBrowser Main component render with selected block 1`] = ` <div className="problem-browser-container" > <div className="problem-browser" > <button className="btn" onBlur={[Function]} onClick={[Function]} onKeyDown={[Function]} type="button" > Select a section or problem </button> <input disabled={true} hidden={false} name="problem-location" type="text" value="some-selected-block" /> <button className="btn" name="list-problem-responses-csv" onBlur={[Function]} onClick={[Function]} onKeyDown={[Function]} type="button" > Create a report of problem responses </button> <div aria-live="polite" className="report-generation-status" /> </div> </div> `;

This test is now only checking for the 'Select a section or problem' button. That is a large change in behavior from before.

To merge this PR this test needs to be updated to test everything it used to be testing.

brian-smith-tcril · 2025-02-05T15:53:16Z

lms/djangoapps/instructor/static/instructor/ProblemBrowser/components/Main/Main.test.jsx

-        expect(fetchCourseBlocksMock.mock.calls.length).toBe(1);
+        const toggleButton = screen.getByRole('button', { name: 'Select a section or problem' });
+        await userEvent.click(toggleButton);
+        expect(fetchCourseBlocksMock).toHaveBeenCalledTimes(1);
    });


🎉 This test does a perfect job of replicating the behavior from before using RTL! 🎉

brian-smith-tcril · 2025-02-05T16:45:44Z

lms/djangoapps/instructor/static/instructor/ProblemBrowser/components/Main/Main.test.jsx

+    test('display dropdown on toggling dropdown', async () => {
+        render(
+            <Provider store={store}>
+                <Main
+                    courseId={courseId}
+                    createProblemResponsesReportTask={jest.fn()}
+                    excludeBlockTypes={excludedBlockTypes}
+                    fetchCourseBlocks={jest.fn()}
+                    problemResponsesEndpoint={problemResponsesEndpoint}
+                    onSelectBlock={jest.fn()}
+                    selectedBlock="some-selected-block"
+                    taskStatusEndpoint={taskStatusEndpoint}
+                    reportDownloadEndpoint={reportDownloadEndpoint}
+                    ShowBtnUi="false"
+                />
+            </Provider>,
+        );
+        expect(screen.queryByTestId('block-browser-container')).toBeNull();
+        const toggleButton = screen.getByRole('button', { name: 'Select a section or problem' });
+        await userEvent.click(toggleButton);
+        await waitFor(() => {
+            expect(screen.findByTestId('block-browser-container')).resolves.toBeInTheDocument();
+        });
+        await userEvent.click(toggleButton);
+        await waitFor(() => {
+            expect(screen.findByTestId('block-browser-container')).resolves.toBeInTheDocument();
+        });
+    });


This test is not properly replicating the behavior from before. The previous test checked for:

expect(component.find(BlockBrowserContainer).length).toBeFalsy(); component.find(Button).find({ label: 'Select a section or problem' }).simulate('click'); expect(component.find(BlockBrowserContainer).length).toBeTruthy();

Given the changes made in this PR to the BlockBrowserContainer component in lms/djangoapps/instructor/static/instructor/ProblemBrowser/components/Main/Main.jsx, it makes sense to consider

screen.findByTestId('block-browser-container')

to be serving the same purpose

component.find(BlockBrowserContainer)

did before.

That being said, before it was testing

In the Enzyme test

expect(component.find(BlockBrowserContainer).length).toBeFalsy(); component.find(Button).find({ label: 'Select a section or problem' }).simulate('click'); expect(component.find(BlockBrowserContainer).length).toBeTruthy();

Before Toggle

expect(component.find(BlockBrowserContainer).length).toBeFalsy();

BlockBrowserContainer IS NOT in document.

After Toggle

expect(component.find(BlockBrowserContainer).length).toBeTruthy();

BlockBrowserContainer IS in document.

In the new RTL test

Before Toggle

expect(screen.queryByTestId('block-browser-container')).toBeNull();

BlockBrowserContainer IS NOT in document.

After (FIRST) Toggle

await waitFor(() => { expect(screen.findByTestId('block-browser-container')).resolves.toBeInTheDocument(); });

BlockBrowserContainer IS in document.

After (SECOND?) Toggle

await waitFor(() => { expect(screen.findByTestId('block-browser-container')).resolves.toBeInTheDocument(); });

BlockBrowserContainer IS (STILL?) in document.

The fact that we are finding an element with the test id block-browser-container even after a second toggle is very concerning to me. The test passing makes me think the test is not properly testing the behavior.

brian-smith-tcril · 2025-02-05T17:03:22Z

lms/djangoapps/instructor/static/instructor/ProblemBrowser/components/Main/Main.test.jsx

+    test('hide dropdown on second toggle', async () => {
+        render(
+            <Provider store={store}>
+                <Main
+                    courseId={courseId}
+                    createProblemResponsesReportTask={jest.fn()}
+                    excludeBlockTypes={excludedBlockTypes}
+                    fetchCourseBlocks={jest.fn()}
+                    problemResponsesEndpoint={problemResponsesEndpoint}
+                    onSelectBlock={jest.fn()}
+                    selectedBlock="some-selected-block"
+                    taskStatusEndpoint={taskStatusEndpoint}
+                    reportDownloadEndpoint={reportDownloadEndpoint}
+                    ShowBtnUi="false"
+                />
+            </Provider>,
        );
-        expect(component.find(BlockBrowserContainer).length).toBeFalsy();
-        component.find(Button).find({ label: 'Select a section or problem' }).simulate('click');
-        expect(component.find(BlockBrowserContainer).length).toBeTruthy();
+        const toggleButton = screen.getByRole('button', { name: 'Select a section or problem' });
+        await userEvent.click(toggleButton);
+        await waitFor(() => {
+            expect(screen.findByText('block-browser-container')).resolves.toBeInTheDocument();
+        });
+        await userEvent.click(toggleButton);
+        await waitFor(() => {
+            expect(screen.queryByText('block-browser-container')).toBeNull();
+        });
    });


This test did not exist before, and is a great addition!

That being said, this implementation is concerning, mostly because it is using findByText and queryByText when looking for a data-testid.

await waitFor(() => { expect(screen.findByText('block-browser-container')).resolves.toBeInTheDocument(); });

The fact that this is passing worries me. findByText should not be finding block-browser-container as that is a data-testid. findByTestId will find elements based on data-testid.

await waitFor(() => { expect(screen.queryByText('block-browser-container')).toBeNull(); });

It does not surprise me that this is passing, but that is because using queryByText to find a data-testid will always be null.

fix: enzyme testing replaced by react tester

3a3b9b1

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jan 23, 2025

fix: ESlint failures

7ff5004

jciasenza mentioned this pull request Jan 23, 2025

fix: enzyme testing replaced by react tester #36026

Closed

feanil requested a review from brian-smith-tcril January 23, 2025 20:29

feanil reviewed Jan 24, 2025

View reviewed changes

package.json Outdated Show resolved Hide resolved

jciasenza and others added 7 commits January 24, 2025 12:20

fix: remove enzyme dep and fix setupTest

dc8557c

Merge branch 'master' into jci/issue35245

16397a1

Merge branch 'master' into jci/issue35245

3cf6c79

Merge branch 'master' into jci/issue35245

3f800ad

Merge branch 'master' into jci/issue35245

60f6746

Merge branch 'master' into jci/issue35245

34b532e

Merge branch 'master' into jci/issue35245

ac8f8a0

jciasenza added 5 commits January 29, 2025 16:45

Merge branch 'master' into jci/issue35245

033c973

Merge branch 'master' into jci/issue35245

4fef66f

Merge branch 'master' into jci/issue35245

1a69f00

Merge branch 'master' into jci/issue35245

b7f10bb

Merge branch 'master' into jci/issue35245

f7a4847

brian-smith-tcril reviewed Jan 31, 2025

View reviewed changes

jciasenza and others added 3 commits February 3, 2025 11:30

fix: enzyme testing replaced by react tester

4b0f48b

Merge branch 'openedx:master' into jci/issue35245

9e9f55f

Merge branch 'jci/issue35245' of https://github.com/jciasenza/edx-pla…

9ace7e8

…tform into jci/issue35245

fix: enzyme testing replaced by react tester

22320b2

brian-smith-tcril reviewed Feb 3, 2025

View reviewed changes

jciasenza added 2 commits February 3, 2025 14:20

fix: enzyme testing replaced by react tester

6dac593

fix: enzyme testing replaced by react tester

9b659e3

brian-smith-tcril reviewed Feb 3, 2025

View reviewed changes

jciasenza and others added 3 commits February 3, 2025 20:13

fix: enzyme testing replaced by react tester

6b855af

fix: enzyme testing replaced by react tester

a757c7b

Merge branch 'master' into jci/issue35245

b68a54c

Merge branch 'master' into jci/issue35245

4f0da14

brian-smith-tcril requested changes Feb 5, 2025

View reviewed changes

		await userEvent.click(toggleButton);

		await userEvent.click(toggleButton);

fix: enzyme testing replaced by react tester #36159

Are you sure you want to change the base?

fix: enzyme testing replaced by react tester #36159

Conversation

jciasenza commented Jan 23, 2025

openedx-webhooks commented Jan 23, 2025 • edited Loading

🔘 Get product approval

🔘 Provide context

🔘 Get a green build

Where can I find more information?

When can I expect my changes to be merged?

jciasenza commented Jan 23, 2025

robrap commented Jan 23, 2025

jciasenza commented Jan 23, 2025

robrap commented Jan 23, 2025

jciasenza commented Jan 23, 2025

robrap commented Jan 23, 2025

jciasenza commented Jan 28, 2025

brian-smith-tcril left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jciasenza commented Feb 3, 2025 • edited Loading

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

brian-smith-tcril commented Feb 4, 2025

jciasenza commented Feb 4, 2025

brian-smith-tcril commented Feb 4, 2025

jciasenza commented Feb 5, 2025

brian-smith-tcril left a comment

Choose a reason for hiding this comment

brian-smith-tcril Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

To merge this PR this test needs to be updated to test everything it used to be testing.

brian-smith-tcril Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

To merge this PR this test needs to be updated to test everything it used to be testing.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

In the Enzyme test

Before Toggle

BlockBrowserContainer IS NOT in document.

After Toggle

BlockBrowserContainer IS in document.

In the new RTL test

Before Toggle

BlockBrowserContainer IS NOT in document.

After (FIRST) Toggle

BlockBrowserContainer IS in document.

After (SECOND?) Toggle

BlockBrowserContainer IS (STILL?) in document.

The fact that we are finding an element with the test id block-browser-container even after a second toggle is very concerning to me. The test passing makes me think the test is not properly testing the behavior.

Choose a reason for hiding this comment

The fact that this is passing worries me. findByText should not be finding block-browser-container as that is a data-testid. findByTestId will find elements based on data-testid.

It does not surprise me that this is passing, but that is because using queryByText to find a data-testid will always be null.

openedx-webhooks commented Jan 23, 2025 •

edited

Loading

jciasenza commented Feb 3, 2025 •

edited

Loading

brian-smith-tcril Feb 5, 2025 •

edited

Loading

brian-smith-tcril Feb 5, 2025 •

edited

Loading

`BlockBrowserContainer` IS NOT in document.

`BlockBrowserContainer` IS in document.

`BlockBrowserContainer` IS NOT in document.

`BlockBrowserContainer` IS in document.

`BlockBrowserContainer` IS (STILL?) in document.

The fact that we are finding an element with the test id `block-browser-container` even after a second toggle is very concerning to me. The test passing makes me think the test is not properly testing the behavior.

The fact that this is passing worries me. `findByText` should not be finding `block-browser-container` as that is a `data-testid`. `findByTestId` will find elements based on `data-testid`.

It does not surprise me that this is passing, but that is because using `queryByText` to find a `data-testid` will always be null.