Skip to content
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

fix: updated the unit tests workflow to simplify job names #29472

Merged
merged 1 commit into from
Dec 9, 2021

Conversation

mraarif
Copy link
Contributor

@mraarif mraarif commented Nov 30, 2021

Description

Updated shard_names in the CI to make shards handling easier.

@mraarif mraarif force-pushed the enhance-sharding-experience branch 3 times, most recently from 5dd343b to 0007fa9 Compare November 30, 2021 13:50
@mraarif mraarif changed the title fix: updated the unit tests workflow to simplify job names and read paths from a json file fix: updated the unit tests workflow to simplify job names Nov 30, 2021
@mraarif mraarif force-pushed the enhance-sharding-experience branch 4 times, most recently from 8ec62db to 4e0820e Compare December 1, 2021 09:11
@mraarif mraarif requested a review from a team December 1, 2021 09:12
@@ -18,7 +18,7 @@ jobs:

steps:
- name: Checkout repo
uses: actions/checkout@v1
uses: actions/checkout@v2
Copy link
Contributor

Choose a reason for hiding this comment

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

Has the codecov issue with v2 been fully resolved? actions/checkout#299 is still open, here's what I wrote the last time we looked into this: openedx/edx-drf-extensions#144 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we initially thought that merge commits failing on master was caused by checkout action but that wasn't the issue. further checkout@v1 take ~1 min to checkout and v2 takes ~4 seconds since we don't yet have coverage issue in edx-platform I think we should switch back to v2
I'm not exactly sure but I can look into it if the coverage issue is resolved or not.

scripts/gha_unit_tests_collector.py Outdated Show resolved Hide resolved
"openedx-1": "openedx/core/djangoapps/ace_common/ openedx/core/djangoapps/cors_csrf/ openedx/core/djangoapps/agreements/ openedx/core/djangoapps/api_admin/ openedx/core/djangoapps/auth_exchange/ openedx/core/djangoapps/bookmarks/ openedx/core/djangoapps/cache_toolbox/ openedx/core/djangoapps/catalog/ openedx/core/djangoapps/ccxcon/ openedx/core/djangoapps/commerce/ openedx/core/djangoapps/common_initialization/ openedx/core/djangoapps/common_views/ openedx/core/djangoapps/config_model_utils/ openedx/core/djangoapps/content/ openedx/core/djangoapps/content_libraries/ openedx/core/djangoapps/contentserver/ openedx/core/djangoapps/cookie_metadata/ openedx/core/djangoapps/course_apps/ openedx/core/djangoapps/course_date_signals/ openedx/core/djangoapps/course_groups/ openedx/core/djangoapps/coursegraph/ openedx/core/djangoapps/courseware_api/ openedx/core/djangoapps/crawlers/ openedx/core/djangoapps/credentials/ openedx/core/djangoapps/credit/ openedx/core/djangoapps/dark_lang/ openedx/core/djangoapps/debug/ openedx/core/djangoapps/demographics/ openedx/core/djangoapps/discussions/ openedx/core/djangoapps/django_comment_common/ openedx/core/djangoapps/embargo/ openedx/core/djangoapps/enrollments/ openedx/core/djangoapps/external_user_ids/",
"openedx-2": "openedx/core/djangoapps/geoinfo/ openedx/core/djangoapps/header_control/ openedx/core/djangoapps/heartbeat/ openedx/core/djangoapps/lang_pref/ openedx/core/djangoapps/models/ openedx/core/djangoapps/monkey_patch/ openedx/core/djangoapps/oauth_dispatch/ openedx/core/djangoapps/olx_rest_api/ openedx/core/djangoapps/password_policy/ openedx/core/djangoapps/plugin_api/ openedx/core/djangoapps/plugins/ openedx/core/djangoapps/profile_images/ openedx/core/djangoapps/programs/ openedx/core/djangoapps/safe_sessions/ openedx/core/djangoapps/schedules/ openedx/core/djangoapps/self_paced/ openedx/core/djangoapps/service_status/ openedx/core/djangoapps/session_inactivity_timeout/ openedx/core/djangoapps/signals/ openedx/core/djangoapps/site_configuration/ openedx/core/djangoapps/system_wide_roles/ openedx/core/djangoapps/theming/ openedx/core/djangoapps/user_api/ openedx/core/djangoapps/user_authn/ openedx/core/djangoapps/util/ openedx/core/djangoapps/verified_track_content/ openedx/core/djangoapps/video_config/ openedx/core/djangoapps/video_pipeline/ openedx/core/djangoapps/waffle_utils/ openedx/core/djangoapps/xblock/ openedx/core/djangoapps/xmodule_django/ openedx/core/djangoapps/zendesk_proxy/ openedx/core/djangolib/ openedx/core/lib/ openedx/core/tests/ openedx/core/tests/ openedx/features/ openedx/testing/ openedx/tests/",
"cms-1": "cms/djangoapps/api/ cms/djangoapps/cms_user_tasks/ cms/djangoapps/course_creators/ cms/djangoapps/export_course_metadata/ cms/djangoapps/maintenance/ cms/djangoapps/models/ cms/djangoapps/pipeline_js/ cms/djangoapps/xblock_config/ cms/envs/ cms/lib/",
"cms-2": "cms/djangoapps/contentstore/",
Copy link
Contributor

@jmbowman jmbowman Dec 2, 2021

Choose a reason for hiding this comment

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

I thought we used to run at least some of the openedx and common tests with both cms and lms settings; are we still doing that, or did that get lost somewhere along the way? https://github.com/edx/edx-platform/blob/master/pavelib/utils/test/suites/pytest_suite.py#L228-L234 . The thinking was to make sure the tests for libraries used in both services pass with the settings currently used for each service.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we have a potential solution for this, running a few more shards that run openedx and common tests with lms and cms settings

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i.e 2x openedx shards with lms settings 2x shards with cms settings, same goes for common

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, please create a ticket for that so we don't lose track of it.

Copy link
Member

Choose a reason for hiding this comment

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

Created Issue https://openedx.atlassian.net/browse/BOM-3103 to keep track of the mentioned CI enhancement.

scripts/unit_test_shards_parser.py Outdated Show resolved Hide resolved
@edx-status-bot
Copy link

Your PR has finished running tests. There were no failures.

"openedx-1": "openedx/core/djangoapps/ace_common/ openedx/core/djangoapps/cors_csrf/ openedx/core/djangoapps/agreements/ openedx/core/djangoapps/api_admin/ openedx/core/djangoapps/auth_exchange/ openedx/core/djangoapps/bookmarks/ openedx/core/djangoapps/cache_toolbox/ openedx/core/djangoapps/catalog/ openedx/core/djangoapps/ccxcon/ openedx/core/djangoapps/commerce/ openedx/core/djangoapps/common_initialization/ openedx/core/djangoapps/common_views/ openedx/core/djangoapps/config_model_utils/ openedx/core/djangoapps/content/ openedx/core/djangoapps/content_libraries/ openedx/core/djangoapps/contentserver/ openedx/core/djangoapps/cookie_metadata/ openedx/core/djangoapps/course_apps/ openedx/core/djangoapps/course_date_signals/ openedx/core/djangoapps/course_groups/ openedx/core/djangoapps/coursegraph/ openedx/core/djangoapps/courseware_api/ openedx/core/djangoapps/crawlers/ openedx/core/djangoapps/credentials/ openedx/core/djangoapps/credit/ openedx/core/djangoapps/dark_lang/ openedx/core/djangoapps/debug/ openedx/core/djangoapps/demographics/ openedx/core/djangoapps/discussions/ openedx/core/djangoapps/django_comment_common/ openedx/core/djangoapps/embargo/ openedx/core/djangoapps/enrollments/ openedx/core/djangoapps/external_user_ids/",
"openedx-2": "openedx/core/djangoapps/geoinfo/ openedx/core/djangoapps/header_control/ openedx/core/djangoapps/heartbeat/ openedx/core/djangoapps/lang_pref/ openedx/core/djangoapps/models/ openedx/core/djangoapps/monkey_patch/ openedx/core/djangoapps/oauth_dispatch/ openedx/core/djangoapps/olx_rest_api/ openedx/core/djangoapps/password_policy/ openedx/core/djangoapps/plugin_api/ openedx/core/djangoapps/plugins/ openedx/core/djangoapps/profile_images/ openedx/core/djangoapps/programs/ openedx/core/djangoapps/safe_sessions/ openedx/core/djangoapps/schedules/ openedx/core/djangoapps/self_paced/ openedx/core/djangoapps/service_status/ openedx/core/djangoapps/session_inactivity_timeout/ openedx/core/djangoapps/signals/ openedx/core/djangoapps/site_configuration/ openedx/core/djangoapps/system_wide_roles/ openedx/core/djangoapps/theming/ openedx/core/djangoapps/user_api/ openedx/core/djangoapps/user_authn/ openedx/core/djangoapps/util/ openedx/core/djangoapps/verified_track_content/ openedx/core/djangoapps/video_config/ openedx/core/djangoapps/video_pipeline/ openedx/core/djangoapps/waffle_utils/ openedx/core/djangoapps/xblock/ openedx/core/djangoapps/xmodule_django/ openedx/core/djangoapps/zendesk_proxy/ openedx/core/djangolib/ openedx/core/lib/ openedx/core/tests/ openedx/core/tests/ openedx/features/ openedx/testing/ openedx/tests/",
"cms-1": "cms/djangoapps/api/ cms/djangoapps/cms_user_tasks/ cms/djangoapps/course_creators/ cms/djangoapps/export_course_metadata/ cms/djangoapps/maintenance/ cms/djangoapps/models/ cms/djangoapps/pipeline_js/ cms/djangoapps/xblock_config/ cms/envs/ cms/lib/",
"cms-2": "cms/djangoapps/contentstore/",
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, please create a ticket for that so we don't lose track of it.

@UsamaSadiq UsamaSadiq merged commit 76a8aaf into master Dec 9, 2021
@UsamaSadiq UsamaSadiq deleted the enhance-sharding-experience branch December 9, 2021 13:21
UsamaSadiq added a commit that referenced this pull request Dec 9, 2021
…d read test paths from a separate file (#29472)"

This reverts commit 76a8aaf.
@edx-pipeline-bot
Copy link
Contributor

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

@edx-pipeline-bot
Copy link
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

1 similar comment
@edx-pipeline-bot
Copy link
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

@edx-pipeline-bot
Copy link
Contributor

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

@edx-pipeline-bot
Copy link
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

@nedbat
Copy link
Contributor

nedbat commented Dec 9, 2021

@UsamaSadiq @mraarif I'd like to cherry-pick the GHA unit tests onto the Maple branch. Can you help me find the correct collection of commits to pick? (for https://openedx.atlassian.net/browse/ARCHBOM-1971)

@iamsobanjaved
Copy link
Contributor

@nedbat
Copy link
Contributor

nedbat commented Dec 10, 2021

I tried those four and found conflicts. We also need 470be08, but there are still conflicts. Would you mind collecting them all together into a pull request against open-release/maple.master?

@nedbat
Copy link
Contributor

nedbat commented Dec 10, 2021

BTW, here is a Jira ticket: https://openedx.atlassian.net/browse/ARCHBOM-1971

iamsobanjaved pushed a commit that referenced this pull request Dec 13, 2021
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.

7 participants