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!

brian-smith-tcril · 2025-01-31T20:28:19Z

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

    });

-    test('render with selected block', () => {
-        const component = renderer.create(
+    test('display dropdown on toggling dropdown', async () => {


It seems this test isn't quite living up to the "display dropdown on toggling dropdown" name.

The only expect I see in here is checking that before the user clicks the toggleButton 'Some expected block name' doesn't appear on the screen.

This should be updated to verify the dropdown is actually displayed when it is toggled.

Check this out

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"

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: enzyme testing replaced by react tester #36159

fix: enzyme testing replaced by react tester #36159

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

brian-smith-tcril Jan 31, 2025

jciasenza Jan 31, 2025

brian-smith-tcril Jan 31, 2025

jciasenza Jan 31, 2025

jciasenza commented Feb 3, 2025 •

edited

Loading

brian-smith-tcril Feb 3, 2025

jciasenza Feb 3, 2025

jciasenza Feb 3, 2025

brian-smith-tcril Feb 3, 2025

jciasenza Feb 3, 2025

brian-smith-tcril Feb 3, 2025

jciasenza Feb 3, 2025

jciasenza Feb 3, 2025

brian-smith-tcril Feb 3, 2025

jciasenza Feb 4, 2025

		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

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

openedx-webhooks commented Jan 23, 2025 •

edited

Loading

jciasenza commented Feb 3, 2025 •

edited

Loading