-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add integration tests for the enrollments api AP-1346
#280
Open
BryanttV
wants to merge
32
commits into
master
Choose a base branch
from
bav/add-integration-tests-to-enrollment-api
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 22 commits
Commits
Show all changes
32 commits
Select commit
Hold shift + click to select a range
e344fd1
feat: add tutor command to import democourse
BryanttV 7a8fe05
chore: add output in tests
BryanttV 46774d4
feat: add openedx organization in fixtures and improve the format
BryanttV 2d68332
test: add enrollment api integration tests
BryanttV c5c25c0
feat: add fixtures for course modes
BryanttV eb43544
test: add remaining integration tests
BryanttV 5ec008a
chore: fix pylint errors
BryanttV 041273a
fix: add missing ddt function in class decorator
BryanttV 50a67dc
chore: move users data to fake_users file
BryanttV 4cdee58
chore: remove utils folder
BryanttV cb0039b
docs: update methods docstring in the tests
BryanttV 7b10550
feat: update matrix in integration tests workflow
BryanttV f8533d7
fix: use correct course_id
BryanttV 9dbce66
fix: add organizations and course modes for course_id of quince
BryanttV 2305410
chore: add test_enrollment_integration file
BryanttV 486246b
chore: add missing letter
BryanttV 843c9b1
refactor: include force field in ddt data when creating enrollment
BryanttV 1e547be
test: add value for each param
BryanttV d6e84e2
docs: update docstring and name of methods
BryanttV 31fada0
chore: add f flag to show a short test summary info
BryanttV a710bcc
refactor: create user in the setup method
BryanttV 05a0e18
chore: remove old enrollments api integrations tests
BryanttV 69692fa
test: check the applications state after each update
BryanttV 658b2fb
chore: remove tutor command to import demo course in shell file
BryanttV 8914e37
ci: update workflow to use new version of integration test in tutor a…
BryanttV a7bae95
chore: use demo_course_id variable name instead course_id
BryanttV 6ba8bb0
chore: avoid extra changes in fixtures
BryanttV 543ab51
Revert "chore: avoid extra changes in fixtures"
BryanttV cb6ce0b
chore: use a string instead of object in fixtures
BryanttV 5a98fd8
ci: add tutor nightly in integration tests workflow
BryanttV 88ee4f3
fix: fix typo
BryanttV c5607d3
chore: update dates in fixtures
BryanttV File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -626,5 +626,57 @@ | |
| "city": "Charleston", | ||
| "goals": "Aenean vulputate eleifend tellus.", | ||
| }, | ||
| { | ||
| "username": "gwalker33", | ||
| "email": "[email protected]", | ||
| "fullname": "Gary Walker", | ||
| "password": "zP7%1Yt!dB@", | ||
| "activate_user": True, | ||
| "mailing_address": "321 Birch Road", | ||
| "year_of_birth": 1996, | ||
| "gender": "m", | ||
| "level_of_education": "hs", | ||
| "city": "Sacramento", | ||
| "goals": "Praesent vestibulum dapibus nibh.", | ||
| }, | ||
| { | ||
| "username": "tturner34", | ||
| "email": "[email protected]", | ||
| "fullname": "Tina Turner", | ||
| "password": "wJ4@5Lm!tQ#", | ||
| "activate_user": False, | ||
| "mailing_address": "753 Pine Street", | ||
| "year_of_birth": 1988, | ||
| "gender": "f", | ||
| "level_of_education": "m", | ||
| "city": "Portland", | ||
| "goals": "Duis lobortis massa nec est.", | ||
| }, | ||
| { | ||
| "username": "jwhite35", | ||
| "email": "[email protected]", | ||
| "fullname": "Jack White", | ||
| "password": "qB2%9Cv!xF@", | ||
| "activate_user": True, | ||
| "mailing_address": "159 Spruce Lane", | ||
| "year_of_birth": 1995, | ||
| "gender": "m", | ||
| "level_of_education": "hs", | ||
| "city": "New Orleans", | ||
| "goals": "Vestibulum volutpat pretium libero.", | ||
| }, | ||
| { | ||
| "username": "gwalker33", | ||
| "email": "[email protected]", | ||
| "fullname": "Gary Walker", | ||
| "password": "zP7%1Yt!dB@", | ||
| "activate_user": True, | ||
| "mailing_address": "321 Birch Road", | ||
| "year_of_birth": 1996, | ||
| "gender": "m", | ||
| "level_of_education": "hs", | ||
| "city": "Sacramento", | ||
| "goals": "Praesent vestibulum dapibus nibh.", | ||
| }, | ||
| ] | ||
| ) | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BryanttV can we please explain why is this implementation necessary in the PR description? I think it's important to explain the reasoning behind this decision
Since I’m considering moving
tutor local do importdemocourseto the GitHub Action, should we also pass the course ID to the environment in the step that runs the integration tests? This way, the action would handle the full demo course setup, and we can ensure that the tests always reference the correct course and prevent each plugin from setting it manuallyI was able to obtain the course ID dynamically with:
COURSE_KEY=$(tutor local exec lms python manage.py lms shell -c "from openedx.core.djangoapps.content.course_overviews.models import CourseOverview; course = CourseOverview.objects.first(); print(course.id, end='')")However, if the import process changes in future Open edX versions, this could break the tests. Of course, we could always introduce version-based logic to handle that, but we could also just set the course ID manually in the action (assuming the Demo course ID won’t change frequently). Starting from Tutor version 18.0.0, the ID is
course-v1:OpenedX+DemoX+DemoCourse, so we’d handle logic for both IDs for now, and remove support for versions below 18.0.0 laterI’d like to hear your thoughts on this approach @BryanttV @mariajgrimaldi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like what you propose. I think it's more appropriate to leave that responsibility to the action, than to keep it in every plugin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should definitely move this to the GH action itself.
I like the proposal's automatic approach to getting the course ID. However, I'm very wary of using code from the platform without any API or interface, even from the shell. I don't think it's worth it, considering the course ID hasn't changed much over time. The last change was very much needed: openedx/openedx-demo-course#50, it looks like it was very axim-specific for some reason.