test: run openedx and common tests with both lms and cms settings by iamsobanjaved · Pull Request #29676 · openedx/openedx-platform

iamsobanjaved · 2021-12-23T14:55:26Z

Previously some tests from openedx and common were running in both lms and cms test suite under Jenkins. But after shifting CI to GHA, those tests were running only with lms settings so now updated that run with both settings.

More details: BOM-3103

nedbat · 2022-01-10T11:28:41Z

-  "common-1": "common/djangoapps/",
-  "common-2": "common/lib/"
+  "lms-1": {
+    "path": ["lms/djangoapps/badges/","lms/djangoapps/branding/","lms/djangoapps/bulk_email/","lms/djangoapps/bulk_enroll/","lms/djangoapps/bulk_user_retirement/","lms/djangoapps/ccx/","lms/djangoapps/certificates/","lms/djangoapps/commerce/"],


Please format this list with one item per line so that diffs will make more sense. (Also, "path" is an odd name for a list of paths?)

current formatting is more readable since it shows which apps are being tested against cms or lms settings, in my opinion, this is easier to understand for developers not aware of the whole sharding strategy
we can change the formatting but we'd have to scroll to the end to see the config for each shard

Sorry, I worded it poorly. I was suggesting it look like this:

{ "lms-1": { "settings": "lms.envs.test", "paths": [ "lms/djangoapps/badges/", "lms/djangoapps/branding/", "lms/djangoapps/bulk_email/", "lms/djangoapps/bulk_enroll/", ..etc.. ] }, "lms-2": { ... }, ... }

nedbat · 2022-01-10T11:30:43Z

-    sys.stdout.write(unit_test_paths)
+    modules = get_unit_test_modules("cms") if argument.cms_only else get_unit_test_modules("lms")
+    paths_output = ' '.join(modules)
+    sys.stdout.write(paths_output)


There's no reason to use sys.stdout.write. Just use print.

nedbat · 2022-01-10T11:31:15Z


-    unit_test_paths = ' '.join(modules)
-    sys.stdout.write(unit_test_paths)
+    modules = get_unit_test_modules("cms") if argument.cms_only else get_unit_test_modules("lms")


Suggested change

modules = get_unit_test_modules("cms") if argument.cms_only else get_unit_test_modules("lms")

modules = get_unit_test_modules("cms" if argument.cms_only else "lms")

nedbat · 2022-01-10T11:32:27Z


-def get_test_paths_for_shard(shard_name):
+def load_unit_test_shards(shard_name):
    unit_tests_json = f'{os.getcwd()}/.github/workflows/unit-test-shards.json'


There's no need to put getcwd into this path, the current directory is the default for opening files.

nedbat · 2022-01-10T11:32:53Z

+def load_unit_test_shards(shard_name):
    unit_tests_json = f'{os.getcwd()}/.github/workflows/unit-test-shards.json'
    with open(unit_tests_json) as file:
        unit_test_workflow_shards = json.loads(file.read())


Suggested change

unit_test_workflow_shards = json.loads(file.read())

unit_test_workflow_shards = json.load(file)

nedbat · 2022-01-10T11:35:17Z

+    output = get_settings_for_shard(argument.shard_name) if argument.output == "settings" \
+        else " ".join(get_test_paths_for_shard(argument.shard_name))
+    sys.stdout.write(output)


Don't use an if expression if you have to split it across lines:

Suggested change

output = get_settings_for_shard(argument.shard_name) if argument.output == "settings" \

else " ".join(get_test_paths_for_shard(argument.shard_name))

sys.stdout.write(output)

if argument.output == "settings":

print(get_settings_for_shard(argument.shard_name))

else:

print(" ".join(get_test_paths_for_shard(argument.shard_name))

nedbat · 2022-01-10T11:37:49Z



-def get_modules_except_cms():
+def get_unit_test_modules(module_name="lms"):


If you always call the function with an explicit argument, it's better to leave off the default.

nedbat · 2022-01-11T17:29:11Z

I was first looking at this code because I had a PR failing because the test counts were wrong. I couldn't figure out how to fix things. Will you be adding some explanation about what to do if the check fails?

nedbat

Thanks for the write-up!

nedbat

One real question: how to decide where to add new apps?

edx-pipeline-bot · 2022-01-21T08:11:32Z

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

edx-pipeline-bot · 2022-01-21T08:52:08Z

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

edx-pipeline-bot · 2022-01-21T08:52:08Z

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

…enedx#29676)

…enedx#29676) (cherry picked from commit 4e22a38)

mraarif force-pushed the iamsobanjaved/BOM-3103 branch 6 times, most recently from 7b0a319 to d6fe637 Compare January 3, 2022 16:46

mraarif requested a review from jmbowman January 3, 2022 16:47

UsamaSadiq approved these changes Jan 4, 2022

View reviewed changes

nedbat suggested changes Jan 10, 2022

View reviewed changes

mraarif force-pushed the iamsobanjaved/BOM-3103 branch 3 times, most recently from b6cce2c to 4fa6cf8 Compare January 11, 2022 11:34

mraarif requested a review from nedbat January 11, 2022 11:52

mraarif force-pushed the iamsobanjaved/BOM-3103 branch from 4fa6cf8 to ca08b63 Compare January 11, 2022 11:54

nedbat reviewed Jan 11, 2022

View reviewed changes

Comment thread scripts/gha_unit_tests_collector.py Outdated

mraarif force-pushed the iamsobanjaved/BOM-3103 branch 2 times, most recently from 6297ffd to f4ecded Compare January 12, 2022 09:15

mraarif requested a review from nedbat January 12, 2022 09:18

UsamaSadiq reviewed Jan 12, 2022

View reviewed changes

mraarif force-pushed the iamsobanjaved/BOM-3103 branch 2 times, most recently from 6b2ed1d to 504596c Compare January 12, 2022 11:03

nedbat suggested changes Jan 12, 2022

View reviewed changes

Comment thread scripts/gha-shards-readme.md Outdated

Comment thread scripts/gha-shards-readme.md Outdated

mraarif force-pushed the iamsobanjaved/BOM-3103 branch from 504596c to 1b4d236 Compare January 12, 2022 15:56

nedbat suggested changes Jan 12, 2022

View reviewed changes

Comment thread scripts/gha-shards-readme.md Outdated

Comment thread scripts/gha-shards-readme.md Outdated

mraarif force-pushed the iamsobanjaved/BOM-3103 branch from 319534a to 2aa13fd Compare January 12, 2022 17:46

nedbat reviewed Jan 12, 2022

View reviewed changes

Comment thread .github/workflows/unit-test-shards.json Outdated

mraarif force-pushed the iamsobanjaved/BOM-3103 branch 2 times, most recently from 226abee to 606d4ab Compare January 17, 2022 17:40

nedbat approved these changes Jan 20, 2022

View reviewed changes

iamsobanjaved and others added 2 commits January 21, 2022 11:29

test: run openedx and common tests with both lms and cms settings

ff58f25

fix: updated the logic for unit test shards and test collection

4700a70

mraarif force-pushed the iamsobanjaved/BOM-3103 branch from 606d4ab to 4700a70 Compare January 21, 2022 06:29

mraarif merged commit 4e22a38 into master Jan 21, 2022

mraarif deleted the iamsobanjaved/BOM-3103 branch January 21, 2022 06:58

ztraboo added a commit to CUCWD/edx-platform that referenced this pull request Apr 7, 2022

test: run openedx and common tests with both lms and cms settings (op…

3fb38dd

…enedx#29676)

Agrendalath pushed a commit to open-craft/openedx-platform that referenced this pull request Jun 7, 2022

test: run openedx and common tests with both lms and cms settings (op…

90420d9

…enedx#29676) (cherry picked from commit 4e22a38)

Agrendalath pushed a commit to open-craft/openedx-platform that referenced this pull request Jun 7, 2022

test: run openedx and common tests with both lms and cms settings (op…

340f3a7

…enedx#29676) (cherry picked from commit 4e22a38)

0x29a mentioned this pull request Sep 21, 2022

[BB-6692] Prepare opencraft-release/nutmeg.2 branch for edx-platform open-craft/openedx-platform#490

Merged

	modules = get_unit_test_modules("cms") if argument.cms_only else get_unit_test_modules("lms")
	modules = get_unit_test_modules("cms" if argument.cms_only else "lms")

	unit_test_workflow_shards = json.loads(file.read())
	unit_test_workflow_shards = json.load(file)



		def get_modules_except_cms():
		def get_unit_test_modules(module_name="lms"):

Uh oh!

Conversation

iamsobanjaved commented Dec 23, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nedbat commented Jan 11, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nedbat left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nedbat left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

edx-pipeline-bot commented Jan 21, 2022

Uh oh!

edx-pipeline-bot commented Jan 21, 2022

Uh oh!

edx-pipeline-bot commented Jan 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants