[GSoC' 24]: Create a wiki page for debugging the new partial ci tests structure. #328

jnvtnguyen · 2024-06-05T19:53:25Z

No description provided.

jnvtnguyen · 2024-06-05T19:55:34Z

HI @seanlip, given this comment, is the name fine for the wiki page fine and should I get this merged before putting information or is it fine to put a link in the error message to a WIKI page that will be created later when I fill this PR.

seanlip · 2024-06-08T15:49:16Z

Hi @jnvtnguyen -- TBH I think this flow is kind of weird, we'll end up with an orphaned wiki page which no one knows what's supposed to go in there :P

Can you fill in this wiki page properly and link it from the sidebar as needed, so that when your PR gets merged, the whole dev flow works correctly (including going to the wiki page and understanding what's going on)? If your PR will go in soon then I think that's all that needs to be done. Otherwise, if it'll take a while more to merge, then don't link this page from the sidebar yet, and file an issue on the wiki repo to do so once PR #XYZ on oppia/oppia is merged.

Does that sound good? Also /cc @vojtechjelinek.

jnvtnguyen · 2024-06-08T17:48:49Z

@seanlip Sounds good to me, PR is close to getting merged, just need @chris7716 to approve. PTAL at the changes, Thanks.

seanlip · 2024-06-08T17:57:48Z

Partial-CI-Tests-Structure.md

@@ -0,0 +1,11 @@
+## Overview
+
+For non-python files, we run partial ci tests depending on the tests that a specific file effects. The process of determining which tests to run goes like this:


effects --> affects

seanlip · 2024-06-08T17:58:23Z

Partial-CI-Tests-Structure.md

@@ -0,0 +1,11 @@
+## Overview


I think this intro is a bit abrupt / lacks context. Could you think through why a developer would reach this page (what will they have been trying to do beforehand) and tailor this page to explain how to do the thing they are seeking to do? A bit like this: https://github.com/oppia/oppia/wiki/If-CI-checks-fail-on-your-PR

Then you can have the other sections as references. But let's make sure that the main part of this is written so that it's easy for developers to do what they need to quickly.

seanlip · 2024-06-08T18:00:19Z

_Sidebar.md

@@ -127,6 +127,7 @@
  * [[Mobile device testing|Mobile-device-testing]]
  * [[Performance testing|Performance-Testing]]
  * [[Build process|Build-process]]
+  * [[Partial CI Tests Structure|Partial-CI-Tests-Structure]]


Should this go under some set of pages that relate to testing?

Done, put it under Testing.

seanlip

Thanks @jnvtnguyen -- took a more detailed pass, PTAL.

seanlip · 2024-06-09T17:15:35Z

Partial-CI-Tests-Structure.md

+
+## Overview of Partial CI Tests Structure
+
+For non-python files, we run partial ci tests depending on the tests that a specific file affects. The process of determining which tests to run goes like this:


Maybe add a sentence after the first one to explain the benefits of doing this.

seanlip · 2024-06-09T17:16:09Z

Partial-CI-Tests-Structure.md

+
+For non-python files, we run partial ci tests depending on the tests that a specific file affects. The process of determining which tests to run goes like this:
+
+1. Generate a root files mapping which maps files to their root files.


Here and below: explain where in the codebase this is stored -- perhaps link to the specific files etc. That makes this much more concrete and developers can examine the code. Currently it's a bit vague.

seanlip · 2024-06-09T17:18:04Z

Partial-CI-Tests-Structure.md

+
+# Root Files Config
+
+The root files config at `core/tests/root-files-config.json` contains all the valid root files that are generated in the root files mapping. This ensures that all files in the codebase either lead to a root file or are a root file themselves. We also have a list of root files that should run all tests, the valid root files are stored in the field `VALID_ROOT_FILES`, while the run all tests root files are stored in the field `RUN_ALL_TESTS_ROOT_FILES`. If you get an error that there is a root file not contained in these lists, please add them to one of these lists depending on the context of the file. If the specific file has a chance to break some part of the Oppia website, like `src/index.html`, please add it to the `RUN_ALL_TESTS_ROOT_FILES`, otherwise if it is a file like `README.md`, please add it to the `VALID_ROOT_FILES`.


Put a period after "We also have a list of root files that should run all tests."

Put a paragraph break before "If you get an error".

After "depending on the context of the file", put a colon, and then have each of the two possible cases be under separate bullet points.

seanlip · 2024-06-09T17:18:31Z

_Sidebar.md

@@ -58,6 +58,7 @@
      * [[End-to-end tests|End-to-End-Tests]]
      * [[Acceptance Tests|Acceptance-Tests]]
      * [[Lighthouse Tests|Lighthouse-Tests]]
+      * [[Partial CI Tests Structure|Partial-CI-Tests-Structure]]


Maybe you could rename this to: "Determining which CI tests run for a PR"

seanlip · 2024-06-09T17:19:23Z

Partial-CI-Tests-Structure.md

@@ -0,0 +1,17 @@
+## Introduction
+
+If your PR fails due to the `check_ci_test_suites_to_run` workflow, please look at the error log and depending on the error, do the following:


Add an intro pargraph: Our CI process has a workflow, check_ci_test_suites_to_run, that determines which test suites to run for a given PR based on the files that that PR modifies.

seanlip · 2024-06-09T17:19:42Z

Partial-CI-Tests-Structure.md

+
+If your PR fails due to the `check_ci_test_suites_to_run` workflow, please look at the error log and depending on the error, do the following:
+
+* If you see an error about the root files config, then see the [root files config section](#root-files-config) below.


There's only one bullet point here ... are there other types of errors? Or should they do something else (catch-all) if they run into some error that's not this one?

Hm, this was the only error that I felt needed explanation, the others can be explained by the overview of this system and the errors are self explanatory.

OK, but a one-bullet-point list is still quite weird. At the very least, you should probably have another bullet point that says something like "If you see any other error, ..."

jnvtnguyen · 2024-06-18T06:18:16Z

FYI do need to merge before this one: oppia/oppia#20435

seanlip

Thanks @jnvtnguyen, did a full pass. PTAL and lmk if you have any questions about the comments.

seanlip · 2024-06-18T16:45:38Z

Partial-CI-Tests-Structure.md

+
+If your PR fails due to the `check_ci_test_suites_to_run` workflow, please look at the error log and depending on the error, do the following:
+
+* If you see an error about the root files config, then see the [root files config section](#root-files-config) below.


OK, but a one-bullet-point list is still quite weird. At the very least, you should probably have another bullet point that says something like "If you see any other error, ..."

seanlip · 2024-06-18T16:46:32Z

Partial-CI-Tests-Structure.md

+
+## Overview of Partial CI Tests Structure
+
+For non-python files, we run partial ci tests depending on the tests that a specific file affects. Running partial tests like this allow for PRs to be checked faster on the tests that a PR actually modifies. This faster test speed allows developers to get feedback faster and allows CI resources to be free more. The process of determining which tests to run goes like this:


ci --> CI

allow for --> allows

on the tests --> using the tests

allows CI resources to be free more --> frees up CI resources

seanlip · 2024-06-18T16:47:17Z

Partial-CI-Tests-Structure.md

+For non-python files, we run partial ci tests depending on the tests that a specific file affects. Running partial tests like this allow for PRs to be checked faster on the tests that a PR actually modifies. This faster test speed allows developers to get feedback faster and allows CI resources to be free more. The process of determining which tests to run goes like this:
+
+1. Generate a root files mapping which maps files to their root files. This file is generated at the start of every PR check run and is stored at `root-files-mapping.json`.
+2. Store golden files containing the page modules that a specific test uses. These golden files are stored at `core/tests/test-modules-mappings/{{test-type}}/{{test-name}}`.


This is actually already part of the codebase, right? If so, mention it in the paragraph above this list (describe these files in the codebase and what they represent) before describing the process, and only have steps 1 and 3 as part of the "process".

Well the PR hasn't been merged yet for the test-modules-mappings and the root-files-mapping is generated for every PR.

Oh, my comment was taking into account that you'd merge the other PR before this one. Could you please read it again with that in mind?

So are you suggesting that I move the 2nd bullet point out (and describe in more in depth with examples)? Though @U8NWXD commented that I should merge a wiki page for Partial-CI-Tests-Structure beforehand, so I suppose I can give references to files that will exist?

Let's have your wiki page refer to the correct eventual locations of files. If a file doesn't exist yet and there's going to be a bit of a gap before it does, add a note to say that it'll be introduced in PR XXX and file an issue on oppia-web-developer-docs to discard the note later. Otherwise it's probably OK to have an inconsistency for a short time (a couple of days), or wait a couple of days to merge the wiki PR.

Thinking about the ideal page, I was pointing out that the process that happens for each PR seems to constitute 1 and 3 only, because 2 is already part of the codebase -- so 2 doesn't really belong with the process that is executed for each PR, which consists of 1 and 3 only. (Or am I misunderstanding how the process works?)

seanlip · 2024-06-18T16:50:00Z

Partial-CI-Tests-Structure.md

+
+# Root Files Config
+
+The root files config at `core/tests/root-files-config.json` contains all the valid root files that are generated in the root files mapping. This ensures that all files in the codebase either lead to a root file or are a root file themselves. We also have a list of root files that should run all tests. The valid root files are stored in the field `VALID_ROOT_FILES`, while the run all tests root files are stored in the field `RUN_ALL_TESTS_ROOT_FILES`.


This is a bit confusing. The root files mapping seems to be generated at the start of each PR check run, but this root files config seems to be a static file in the codebase. Wouldn't the root files mapping differ for each PR, and if so, how can its files match the static file all the time?

Also, as a general note, you can use hyphenation or quotes to make some of the "idea boundaries" clearer, e.g. "root-files config" and "run-all-tests root files" or "run all tests" root files.

Sorry, can you elaborate on your first paragraph. The root files config is a way to validate that all root files are captured, so if a PR adds a new root file, there should be an error as we compare the dynamic root files mapping with the root files config and if it isn't there, the developer should add it to VALID_ROOT_FILES or RUN_ALL_TESTS_ROOT_FILES.

You say, above, "Generate a root files mapping which maps files to their root files. This file is generated at the start of every PR check run." That's what I'm referring to at the start of my first paragraph.

Then you have this "root file config", which seems to be a static file. The paragraph here says "The root files config at core/tests/root-files-config.json contains all the valid root files that are generated in the root files mapping." which implies that the files in root-files-mapping and root-files-config match. Did you mean to say something like "the root files config contains a list of all the valid root files, and the root files mapping generated in each PR is a subset of this list"? (Also not sure what "valid" means here -- I took it to mean "exists in the codebase" but if you had a different meaning for it like below, that might be worth clarifying too.)

The root-files-config.json and the root files mapping should match or the check will give an error and therefore the developer should update the root-files-config.json so they do match. Valid means that it is a root file that should have no parents (README, page modules, etc.). Most of the time the root-files-config.json will contain the same valid root files as the root files mapping, as a developer isn't adding any new pages or files (that aren't referenced by another file).

Given this, I think I can explain more about how the root-files-mapping and root-files-config can differ and in this case it will give an error?

Yes, I think that might help. I also wonder if it's better to find a name that's not "valid" as it's not clear to a general developer what "valid" means in this context.

seanlip · 2024-06-18T16:51:56Z

Partial-CI-Tests-Structure.md

+If you get an error that there is a root file not contained in these lists, please add them to one of these lists depending on the context of the file:
+
+* If the specific file has a chance to break some part of the Oppia website, like `src/index.html`, please add it to the `RUN_ALL_TESTS_ROOT_FILES`.
+* If it is a file like `README.md` which doesn't affect any test, please add it to the `VALID_ROOT_FILES`.


I think "VALID" is not clear naming, it is not clear that "RUN ALL TESTS ROOT FILES" are "invalid" in some way. Perhaps give this a clearer name, e.g. RUN_NO_TESTS_ROOT_FILES?

I think that is a better name, in that case which PR should I add it to?

Whichever PR introduces VALID_ROOT_FILES -- let's fix that name. (Or a separate PR if VALID_ROOT_FILES is already in develop.)

Its already in develop.

OK, suggest making a separate PR for this change, then.

jnvtnguyen · 2024-06-20T17:51:46Z

@seanlip Updated, PTAL. Thanks.

seanlip

Thanks @jnvtnguyen, just minor comments.

Do you want this PR merged right after you fix those, or should we wait?

seanlip · 2024-06-21T08:09:23Z

Partial-CI-Tests-Structure.md

+
+# Root Files Config
+
+The root files config at `core/tests/root-files-config.json` contains all the valid root files that are generated in the root files mapping. A root file is a file in the dependency graph which has no parents, the most common root files in the codebase are page modules and other files like `README.md`. The "root files mapping generator" validates the `root-files-config.json` by comparing the PR's root files in the mapping with the root files found in the config, if there are any discrepancies, the generator will error. This ensures that the "root-files config" is always up to date with every PR.


...in the config. If there are ...

... will error; this ensures ...

seanlip · 2024-06-21T08:10:12Z

Partial-CI-Tests-Structure.md

+# Root Files Config
+
+The root files config at `core/tests/root-files-config.json` contains all the valid root files that are generated in the root files mapping. A root file is a file in the dependency graph which has no parents, the most common root files in the codebase are page modules and other files like `README.md`. The "root files mapping generator" validates the `root-files-config.json` by comparing the PR's root files in the mapping with the root files found in the config, if there are any discrepancies, the generator will error. This ensures that the "root-files config" is always up to date with every PR.
+If you get an error that there is a root file not contained in these lists, please add them to one of these lists depending on the context of the file:


Put another blank line above this, it can be a separate paragraph.

seanlip · 2024-06-21T08:10:37Z

Partial-CI-Tests-Structure.md

+The root files config at `core/tests/root-files-config.json` contains all the valid root files that are generated in the root files mapping. A root file is a file in the dependency graph which has no parents, the most common root files in the codebase are page modules and other files like `README.md`. The "root files mapping generator" validates the `root-files-config.json` by comparing the PR's root files in the mapping with the root files found in the config, if there are any discrepancies, the generator will error. This ensures that the "root-files config" is always up to date with every PR.
+If you get an error that there is a root file not contained in these lists, please add them to one of these lists depending on the context of the file:
+
+* If the specific file has a chance to break some part of the Oppia website, like `src/index.html`, please add it to the `RUN_ALL_TESTS_ROOT_FILES`.


...to the RUN_ALL_TESTS_ROOT_FILES file.

Ditto below.

I put to the RUN_ALL_TESTS_ROOT_FILES in root-files-config.json

jnvtnguyen · 2024-06-21T13:33:01Z

@seanlip Done, preferably after fixes since I still need to merge the other PR, Thanks.

seanlip

LGTM. Feel free to request merge whenever you like, thanks!

jnvtnguyen · 2024-06-21T16:43:53Z

Oh sorry @seanlip, I meant preferably after the fixes for this PR (so now), so I can merge the other PR.

seanlip · 2024-06-21T16:51:04Z

Ah ok! Doing that now :)

Updates

adf10bf

jnvtnguyen changed the title ~~[GSoC' 24]: Create a wiki page for debugging the new partical ci tests structure.~~ [GSoC' 24]: Create a wiki page for debugging the new partial ci tests structure. Jun 5, 2024

jnvtnguyen mentioned this pull request Jun 5, 2024

[GSoC'24] M1.1,M1.2: Create a script which generates a mapping of files and their root files. oppia/oppia#20362

Merged

5 tasks

seanlip assigned jnvtnguyen Jun 8, 2024

Updates

7bdd170

jnvtnguyen requested a review from seanlip June 8, 2024 17:48

seanlip requested changes Jun 8, 2024

View reviewed changes

jnvtnguyen and others added 2 commits June 8, 2024 23:29

Updates

85873ff

Merge branch 'develop' into partial-ci-tests-structure

f2fe452

seanlip requested changes Jun 9, 2024

View reviewed changes

jnvtnguyen added 3 commits June 17, 2024 14:23

Updates

64b0a40

Updates

8a8184a

Updates

b8e2c88

jnvtnguyen assigned seanlip and unassigned jnvtnguyen Jun 18, 2024

seanlip requested changes Jun 18, 2024

View reviewed changes

seanlip assigned jnvtnguyen and unassigned seanlip Jun 18, 2024

Updates

8983299

jnvtnguyen mentioned this pull request Jun 18, 2024

Change VALID_ROOT_FILES to RUN_NO_TESTS_ROOT_FILES in core/tests/root-files-config.json. oppia/oppia#20498

Merged

Updates

03d8149

jnvtnguyen assigned seanlip Jun 20, 2024

seanlip requested changes Jun 21, 2024

View reviewed changes

seanlip removed their assignment Jun 21, 2024

Updates

97267e1

jnvtnguyen assigned seanlip and unassigned jnvtnguyen Jun 21, 2024

seanlip approved these changes Jun 21, 2024

View reviewed changes

seanlip assigned jnvtnguyen and unassigned seanlip Jun 21, 2024

jnvtnguyen assigned seanlip and unassigned jnvtnguyen Jun 21, 2024

seanlip merged commit b3699bc into oppia:develop Jun 21, 2024
3 checks passed

		@@ -0,0 +1,11 @@
		## Overview

		For non-python files, we run partial ci tests depending on the tests that a specific file effects. The process of determining which tests to run goes like this:


		## Overview of Partial CI Tests Structure

		For non-python files, we run partial ci tests depending on the tests that a specific file affects. The process of determining which tests to run goes like this:


		For non-python files, we run partial ci tests depending on the tests that a specific file affects. The process of determining which tests to run goes like this:

		1. Generate a root files mapping which maps files to their root files.


		# Root Files Config

		The root files config at `core/tests/root-files-config.json` contains all the valid root files that are generated in the root files mapping. This ensures that all files in the codebase either lead to a root file or are a root file themselves. We also have a list of root files that should run all tests, the valid root files are stored in the field `VALID_ROOT_FILES`, while the run all tests root files are stored in the field `RUN_ALL_TESTS_ROOT_FILES`. If you get an error that there is a root file not contained in these lists, please add them to one of these lists depending on the context of the file. If the specific file has a chance to break some part of the Oppia website, like `src/index.html`, please add it to the `RUN_ALL_TESTS_ROOT_FILES`, otherwise if it is a file like `README.md`, please add it to the `VALID_ROOT_FILES`.

		@@ -0,0 +1,17 @@
		## Introduction

		If your PR fails due to the `check_ci_test_suites_to_run` workflow, please look at the error log and depending on the error, do the following:


		If your PR fails due to the `check_ci_test_suites_to_run` workflow, please look at the error log and depending on the error, do the following:

		* If you see an error about the root files config, then see the [root files config section](#root-files-config) below.

[GSoC' 24]: Create a wiki page for debugging the new partial ci tests structure. #328

[GSoC' 24]: Create a wiki page for debugging the new partial ci tests structure. #328

Conversation

jnvtnguyen commented Jun 5, 2024

jnvtnguyen commented Jun 5, 2024

seanlip commented Jun 8, 2024

jnvtnguyen commented Jun 8, 2024

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

seanlip left a 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

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

jnvtnguyen commented Jun 18, 2024

seanlip left a 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

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

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

jnvtnguyen commented Jun 20, 2024

seanlip left a 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

jnvtnguyen commented Jun 21, 2024

seanlip left a comment

Choose a reason for hiding this comment

jnvtnguyen commented Jun 21, 2024

seanlip commented Jun 21, 2024