Skip to content

test: fix team tests for multiple courses - #32638

Merged
robrap merged 2 commits into
masterfrom
robrap/restore-teams-test
Jul 7, 2023
Merged

test: fix team tests for multiple courses#32638
robrap merged 2 commits into
masterfrom
robrap/restore-teams-test

Conversation

@robrap

@robrap robrap commented Jul 3, 2023

Copy link
Copy Markdown
Contributor

Description

  1. Revert deletion of the original test code.
  2. The original tests looked like helpers, but we are guessing that they were supposed to be tests. Adjusted so that they are actual tests.
  3. Additionally, one test had a small bug as written. It's uglier now, but it passes.

Note to reviewer: Reviewing the test commit 0ccf15d makes it easier to see what changes I made.

  • UPDATE: I then had to move one of the tests, which is in a follow-up commit.

This reverts commit ad3b561.

Follow-up commit will convert these helpers to actual
tests so they are run.

@kdmccormick kdmccormick left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, and thank you!

Non-blocking, but consider adding the link to the original commit or PR (#10804) to your commit message, just to tie things together more for those that weren't in the Slack thread.

# Check that list of user teams in course two is still empty
course_two_teams_url = reverse('teams_dashboard', args=[course_two.id])
response = self.client.get(course_two_teams_url)
self.assertContains(response, '"teams": {"next": null, "previous": null, "count": 0')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are doing substring-matching on JSON, right? It would be more robust to parse the JSON and query it, but up to you. (More important to restore these than to make them totally robust.)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm leaving this for the next person who wants to improve. Agree it would be nice.

The original tests looked like helpers,
but we are guessing that they were supposed
to be tests.

Additionally, one test had a small bug as written.
It would be more resilient to actually parse the
JSON, but that work is being left for a later time.
For now, it works, but is not resilient.

The original PR can be found here:
#10804
@robrap
robrap force-pushed the robrap/restore-teams-test branch from 76ae069 to 0a4bc11 Compare July 7, 2023 13:35
@robrap
robrap merged commit 6d6893c into master Jul 7, 2023
@robrap
robrap deleted the robrap/restore-teams-test branch July 7, 2023 15:08
@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

1 similar comment
@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants