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.

🔘 Update the status of your PR

Your PR is currently marked as a draft. After completing the steps above, update its status by clicking "Ready for Review", or removing "WIP" from the title, as appropriate.

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.

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.

FYI just because tests pass doesn't mean that they're correct. It doesn't make logical sense that you'd be testing that something is null both before and after a click. That seems like it's testing for the wrong thing.

Yes, thanks sarina, I am still working on the issues that Brian told me about, I hope to solve them and I can confirm, I have already made other PRs that were merged, but well, this one is causing me more conflicts.

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.

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

jciasenza · 2025-02-11T15:12:58Z

I returned the previous tests as they were and changed only the one that used Enzyme

Main Differences
Rendering Method
First test uses render() from @testing-library/react, which mounts the full component in an environment similar to a real browser.
Second test uses shallow() from Enzyme, which only renders the top-level component without its child components (shallow rendering).
Interactions with the DOM
First test interacts with the button using userEvent.click() and verifies DOM changes with screen.findByTestId().
Second test uses simulate('click') on a button inside the Main component and checks for the appearance/disappearance of BlockBrowserContainer in the React tree.
Handling Asynchronous Tests
First test uses await waitFor() to handle asynchronous DOM updates, which is crucial if the component has useEffect or state updates.
Second test is entirely synchronous and assumes that component changes happen immediately after simulate('click').
Checking Dropdown Visibility
First test uses screen.queryByTestId() and screen.findByTestId() to check if the element exists in the DOM.
Second test uses component.find(BlockBrowserContainer).length to count whether the component was rendered or not.

sarina · 2025-02-11T18:26:51Z

@angonz could we get your eyes (or another Aulasneo dev) on this PR? Brian's outstanding comments aren't addressed - we need these tests refactored in such a way that they need to test what they were testing previously. Brian has explained why this is the case and what is wrong.

@jciasenza could you please put this PR in draft until your team is ready for review of tests that match what the snapshots were previously testing? Thank you!

sarina · 2025-02-12T18:30:08Z

@jciasenza see the following guides about PR tests, it'll explain why "Lint Commit Messages" is failing and gives more detail about reading build output.

The terrain app and its stub servers were removed as a part of openedx#36102 but these last few things got missed.

The CourseQualityView used to call edx-val's get_video_for_course(), which would return a fully serialized data structure that included all encodings and inefficiently serialized them with many n+1 queries. This is tolerable in a paginated web view, but not when pulling all of a large courses's videos at once. Making this change collapsed the number of queries for a large sample MIT course from over 3000 down to 1.

version bump Commit generated by workflow `openedx/edx-platform/.github/workflows/upgrade-one-python-dependency.yml@refs/heads/master`

fix: enterprise-customer-support should only return learners from given customer Commit generated by workflow `openedx/edx-platform/.github/workflows/upgrade-one-python-dependency.yml@refs/heads/master`

This addresses an issue where get_schedule_for_user was making an excessive number of calls to the database.

chore: upgrade faker package version Commit generated by workflow `openedx/edx-platform/.github/workflows/upgrade-one-python-dependency.yml@refs/heads/master` Co-authored-by: UsamaSadiq <41958659+UsamaSadiq@users.noreply.github.com> Co-authored-by: Usama Sadiq <usama.sadiq@arbisoft.com>

Co-authored-by: Hassan Raza <h.raza@192.168.1.12>

Commit generated by workflow `openedx/edx-platform/.github/workflows/upgrade-one-python-dependency.yml@refs/heads/master` Co-authored-by: UsamaSadiq <41958659+UsamaSadiq@users.noreply.github.com> Co-authored-by: Usama Sadiq <usama.sadiq@arbisoft.com>

Adds api for listing upstream entity links for a given course.

chore: Update xblocks-contrib pypi package Commit generated by workflow `openedx/edx-platform/.github/workflows/upgrade-one-python-dependency.yml@refs/heads/master`

…x#35612) * feat!: upgrading api to DRF

…6199) * fix: Render Word cloud and conditional block editor - The xmodule-type to render is MetadataOnlyEditingDescriptor - The xmodule type `MetadataOnlyEditingDescriptor` renders a `<div>` with the block metadata in the `data-metadata` attribute. But is necessary to call `XBlockEditorView.xblockReady()` to run the scripts to build the editor using the metadata. - To call XBlockEditorView.xblockReady() we need a specific require.config * fix: Adding save and cancel button * fix: save with studio_submit of conditional_block and word_cloud_block * test: Tests for studio_submit of conditional and word cloud * revert: Delete studio_submit of conditional block. It is not supported * style: Fix lint --------- Co-authored-by: Navin Karkera <navin@opencraft.com>

…edx#35852)" This reverts commit 9274852.

Co-Authored-By: Feanil Patel <feanil@axim.org>

These app models get pulled in when trying to build the OpenAPI docs so we need their relevant apps installed into the environment used to generate the docs.

The format of the `source_suffix` setting has change but the default is fine for us so we don't need to override this.

Commit generated by workflow `openedx/edx-platform/.github/workflows/upgrade-one-python-dependency.yml@refs/heads/master`

…onsistent This commit fixes an inconsistency in the way an exam due date is computed for courses that do not use an LTI based proctoring provider. The edx-exams microservice was released last year to provide support for LTI based proctoring providers. After the release of this microservice, all proctoring requests initiated by the platform began to be funneled through the microservice, which acted as a broker for these requests, routing them directly to the service for exams in courses using an LTI based proctoring provider or to the platform edx-proctoring plugin for all other cases. There is an asynchronous task in the platform that syncs exams from the platform to either the edx-exams microservice or the edx-proctoring plugin. Prior to the release of the microservice, this task computed the due date on exams as the exam subsection due date if the course was instructor-paced or None. After the release of the microservice, the task computed due dates differently than before. The due date on exams was computed as the due date on the exam, if there was one, or the end date of the course, if there was one, or None. This differed from the prior definition. This resulted in inconsistent due date behavior. The exams in courses that were published or republished after the edx-exams microservice was released had the new computation of due date, while exams in courses that were published or republished before the edx-exams microservice was released had the old computation of due date. This causes an issue for all exams in courses using non-LTI based providers. This is because the due date on exams across all courses that either do not use proctoring or that use a non-LTI based provider are inconsistent, depending on when they were last published. This commit reintroduces the old computation to the task for exams in courses using a non-LTI based proctoring provider (i.e. those courses whose exams are not powered by the edx-exams microservice). In order to maintain the functionality of edx-exams, we continue to compute the due date as before for exams in courses using an LTI based proctoring provider (i.e. those courses whose exams are powered by the edx-exams microservice).

Adds logic to support the functionality of editing xblocks via the legacy modal editing window.

* docs: fixing make docs command.

…36109) Correctly name authoring subdomain according to DDD docs: https://openedx.atlassian.net/wiki/spaces/AC/pages/663224968/edX+DDD+Bounded+Contexts

* Add error handler on save video to avoid creating sjson * Support transcripts without edx_video_id in definition_to_xml * When copying a video from a library to a course: Create a new edx_video_id * Save transcripts as static assets in a video in a library when adding a new transcript. * Delete transcripts as static assets in a video in a library when deleting transcripts. * Support download transcript in a video in a library. * Support replace transcript in a video in a library. * Support updating transcripts in video in a library. * Refactor the code of downloading YouTube transcripts to enable this feature in libraries. * Support copy from a library to a course and a course to a library.

…penedx#36287) for Open edX operators who still have users with legacy PDF certificates, retirement requires first extracting information from the user's GeneratedCertificate record in order to delete the 4 associated files for each PDF certificate, and then removing the links to the relevant files. this creates a management command to do that work. After thinking about it, I have removed the update to `status` from this management command, as per the original specification of the ticket. I added it for completeness originally, but was already uncomfortable, because it's not exactly accurate. The `CertificateStatuses` enum does define a `deleted` status: ``` deleted - The PDF certificate has been deleted. ``` but I think it's inappropriate to use here. #### Why not use `CertificateStatuses.deleted` in the first place There are multiple places in the code where it's clear that almost all of the statuses are legacy and unused (eg. [Example 1](https://github.com/openedx/edx-platform/blob/6c6fd84e533e6cb272da2e618f227225ad907232/lms/djangoapps/certificates/data.py#L12-L34), [Example 2](https://github.com/openedx/edx-platform/blob/1029de5537752ecf0477d578dba8a932e741e497/common/djangoapps/student/helpers.py#L491-L492)). There are innumerable APIs in the system that have expectations about what might possibly be returned from a `GeneratedCertificate.status` object, and none of them is expecting `deleted` #### Why not revoke the certificate Ultimately, the certificate isn't revoked, which has a specific meaning around saying it was unearned. The certificate was earned; it has simply been deleted. We should not be kicking off program certificate invalidation, because that's not what's happening. We should be trusting the normal user retirement process to remove/purge PII from any program certificates that might exist. The nature of web certificates simply means that we are going through this process outside of the normal retirement flow. The normal retirement flow can be trusted to implement any certificate object revocation/removal/PII-purging, and doing an extra step outside of that flow is counterproductive. #### Why not robustly add a flow for `CertificateStatuses.deleted` When PDF certificates were removed from the system, they weren't removed in their entirety. Instead, we have this vestigial remains of PDF certificates code, just enough to allow learners to display and use the ones that they already have, without any of the support systems for modifying them. Adding a `deleted` status, verifying that all other APIs wouldn't break in the presence of a certificate with that status, adding the signals to process and propagate the change: all of this would be adding more tech debt upon the already existing technical debt which is the PDF certs code. Better to simply add this one necessary data integrity change, and focus on a process which might allow us to eventually remove the web certificates code. #### Why it is good enough to ignore the status The original ask was simply to enforce data integrity: to remove links to files that have been deleted, as an indication that they've been deleted. I only added `status` update out of a (misplaced but well-intentioned) completionist urge. FIXES: APER-3889

…enedx#36181) fix: reformulation of the Main component test fix: reformulation of the Main component test fix: reformulation of the Main component test fix: reformulation of the Main component test fix: reformulation of the Main component test fix: reformulation of the Main component test fix: reformulation of the Main component test fix: enzyme testing replaced by react tester fix: enzyme testing replaced by react tester fix: enzyme testing replaced by react tester

…etting envs (openedx#36296) * chore: add TRANSCRIPT_LANG_CACHE_TIMEOUT setting variable in common setting envs * chore: bump edx-val version fix: enzyme testing replaced by react tester fix: enzyme testing replaced by react tester fix: enzyme testing replaced by react tester fix: enzyme testing replaced by react tester fix: enzyme testing replaced by react tester

jciasenza · 2025-02-26T01:51:36Z

Hello @feanil, @brian-smith-tcril and @sarina : I have decided to close this PR, and after carefully studying the problems I have had with it, I have decided to start a new PR where I explain everything I have done. I hope that everything requested now works.
According to what I discussed with my team, it seemed like the most logical thing to do after the errors I had after the updates.
A thousand apologies and thanks for reviewing it. #36303

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

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

brian-smith-tcril reviewed Jan 31, 2025

View reviewed changes

brian-smith-tcril reviewed Feb 3, 2025

View reviewed changes

brian-smith-tcril requested changes Feb 5, 2025

View reviewed changes

jciasenza marked this pull request as draft February 12, 2025 18:58

feanil and others added 5 commits February 25, 2025 15:19

fix: Dropping terrain related config and docs.

39a3e64

The terrain app and its stub servers were removed as a part of openedx#36102 but these last few things got missed.

feat: Upgrade Python dependency edx-enterprise

8cb6a62

version bump Commit generated by workflow `openedx/edx-platform/.github/workflows/upgrade-one-python-dependency.yml@refs/heads/master`

feat: Upgrade Python dependency edx-enterprise

abd8a86

fix: enterprise-customer-support should only return learners from given customer Commit generated by workflow `openedx/edx-platform/.github/workflows/upgrade-one-python-dependency.yml@refs/heads/master`

perf: update edx-when to 2.5.1 for performance fix

2a57d77

This addresses an issue where get_schedule_for_user was making an excessive number of calls to the database.

eemaanamir and others added 25 commits February 25, 2025 15:21

fix: fixed escaped characters in goal reminder email (openedx#36260)

fe47067

fix: tracker context issue (openedx#36247)

b567f5d

Co-authored-by: Hassan Raza <h.raza@192.168.1.12>

feat: entity link view and api (openedx#36190)

005abd5

Adds api for listing upstream entity links for a given course.

feat: Upgrade Python dependency xblocks-contrib

6ca0c54

chore: Update xblocks-contrib pypi package Commit generated by workflow `openedx/edx-platform/.github/workflows/upgrade-one-python-dependency.yml@refs/heads/master`

feat!: upgrading get_proctored_exam_results api to DRF ( 32 ) (opened…

d246d20

…x#35612) * feat!: upgrading api to DRF

fix: log only for allowed_enroll email (openedx#36273)

46f813a

Revert "fix: Remove pointless Maintenance and Announcement apps (open…

f3ef2fe

…edx#35852)" This reverts commit 9274852.

refactor: Remove broken Force-Publish UI from Studio

1743b49

Co-Authored-By: Feanil Patel <feanil@axim.org>

build: Add more apps to the set of installed apps.

f83821e

These app models get pulled in when trying to build the OpenAPI docs so we need their relevant apps installed into the environment used to generate the docs.

build: We don't need to override this setting.

52329c3

The format of the `source_suffix` setting has change but the default is fine for us so we don't need to override this.

docs: Update the Open API docs.

76ff3c9

feat: Upgrade Python dependency xblock-drag-and-drop-v2

8905fcb

Commit generated by workflow `openedx/edx-platform/.github/workflows/upgrade-one-python-dependency.yml@refs/heads/master`

feat: course unit - edit iframe modal window (openedx#35777)

175c0a5

Adds logic to support the functionality of editing xblocks via the legacy modal editing window.

Merge upstream into my branch

a0b4c13

docs: fixing make docs command. (openedx#36280)

ace0c2f

* docs: fixing make docs command.

fix: import authoring filter from content_authoring instead (openedx#…

fab5d8b

…36109) Correctly name authoring subdomain according to DDD docs: https://openedx.atlassian.net/wiki/spaces/AC/pages/663224968/edX+DDD+Bounded+Contexts

fix: send correct status for scanning

fc9c5c2

jciasenza force-pushed the jci/issue35245 branch from ada9a32 to 38dd757 Compare February 25, 2025 18:22

jciasenza added 2 commits February 25, 2025 15:26

Merge branch 'openedx:master' into jci/issue35245

9220d86

Merge branch 'openedx:master' into jci/issue35245

d71b46f

jciasenza closed this Feb 26, 2025

		await userEvent.click(toggleButton);

		await userEvent.click(toggleButton);

fix: enzyme testing replaced by react tester #36159

fix: enzyme testing replaced by react tester #36159

Conversation

jciasenza commented Jan 23, 2025

openedx-webhooks commented Jan 23, 2025 • edited Loading

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

sarina Feb 12, 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

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.

jciasenza commented Feb 11, 2025

sarina commented Feb 11, 2025

sarina commented Feb 12, 2025

jciasenza commented Feb 26, 2025 • edited Loading

openedx-webhooks commented Jan 23, 2025 •

edited

Loading

jciasenza commented Feb 3, 2025 •

edited

Loading

sarina Feb 12, 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.

jciasenza commented Feb 26, 2025 •

edited

Loading