From 69bb84b7f9027682b080de19f15987854af9440e Mon Sep 17 00:00:00 2001 From: "Kyle D. McCormick" Date: Tue, 29 Oct 2024 17:15:11 -0400 Subject: [PATCH] docs: ADR for Fixing Quality and JS Checks --- .../0021-fixing-quality-and-js-checks.rst | 139 ++++++++++++++++++ 1 file changed, 139 insertions(+) create mode 100644 docs/decisions/0021-fixing-quality-and-js-checks.rst diff --git a/docs/decisions/0021-fixing-quality-and-js-checks.rst b/docs/decisions/0021-fixing-quality-and-js-checks.rst new file mode 100644 index 000000000000..ca7c0ec42a12 --- /dev/null +++ b/docs/decisions/0021-fixing-quality-and-js-checks.rst @@ -0,0 +1,139 @@ +Fixing the Quality and JS checks +################################ + +Status +****** + +Accepted + +To be implemented in https://github.com/openedx/edx-platform/pull/35159 + +Context +******* + +edx-platform PRs need to pass a series of CI checks before merging, including +but not limited to a CLA check, Python unit tests, and Pylint. Of these checks, +two checks were implemented using the "Paver" Python package, a scripting +library `which we have been trying to move off of`_. These two checks are each +built of steps. The checks and their steps are: + +* **Quality others** + + * **pii_check**: Ensure that Django models have PII annotations as + described in `OEP-30`_, with a minimum threshold of **94.5%** of models + annotated. + * **stylelint**: Statically check sass stylesheets for common errors. + * **pep8**: Run pycodestyle against Python code. + * **eslint**: Statically check javascript code for common errors. + * **xsslint**: Check python & javascript for xss vulnerabilities. + * **check_keywords**: Compare Django model field names against a denylist of + reserved keywords. + +* **JS** + + * **test-js**: Run javascript unit tests. + * **coverage-js**: Check that javascript test coverage has not dropped. + +As we work to reimplement these checks without Paver, we have unfortunately +noticed that three of those steps have bugs in their implementations, and thus +have not been enforcing anything for some undetermined amount of time: + +* **pii_check**: Instead of just checking the result of the underlying + code_annotations command, this check wrote an annotations report to a file, + and then used regex to parse the report and determine whether the check + should pass. However, the check failed to validate that the generation of the + report itself was successful. So, when malformed annotations were introduced + to the edx-proctoring repository, which edx-platform installs, the check + began silently passing. + +* **stylelint**: At some point, the `stylelint` binary stopped being available + on the runner's `$PATH`. Rather than causing the Quality Others check to + fail, the Paver code quietly ignored the shell error, and considered the + empty stylelint report file to indicate that there were not linting + violations. + +* **coverage-js**: This check tried to use `diff-cover` in order to compare the + coverage report on the current branch with the coverage report on the master + branch. However, the coverage report does not exist on the master branch, and + it's not clear when it ever did. The coverage-js step failed to validate that + `diff-cover` ran successfully, and instead of raising an error, it allowed + the JS check to pass. + +Decision & Consequences +*********************** + +pii_check +========= + +The maintainers of the edx-proctoring repository will `fix the malformed +annotations`_, allowing the pii_check to once again check model coverage. We +will ensure that any failure of the code_annotations command (due to, for +example, future malformed annotations) will cause the pii_check step and the +overall Quality Others check to fail. We will stop trying to parse the result of +the annotations report in CI, as this was and is completely unneccessary. + +In order to keep "Quality others" passing on the edx-platform master branch, we +will lower the PII annotation coverage threshold to reflect the percentage of +currently-annotated models: **71.6%**. + +We will commit some timeboxed effort into understanding what models are now +missing annotations and what remediations can be taken to raise the threshold +closer to its old value, although it is unlikely that this effort will return +us to 94.5%. + +This is all already `announced on the forums`_. + +stylelint +========= + +We will remove the **stylelint** step entirely from the "Quality Others" check. +Sass code in the edx-platform repository will no longer be subject to any +static analysis. + +coverage-js +=========== + +We will remove the **coverage-js** step entirely from the "JS" check. +JavaScript code in the edx-platform repository will no longer be subject to any +unit test coverage checking. + +Rejected Alternatives +********************* + +* While it would be ideal to raise the pii_check threshold to 94.5% or even + 100%, we do not have the resources to promise this. + +* It would also be nice to institute a "racheting" mechanism for the PII + annotation coverage threshold. That is, every commit to master could save the + coverage percentage to a persisted artifact, allowing subsequent PRs to + ensure that the pii_check never returns lower than the current threshold. We + will put this in the Aximprovements backlog, but we cannot commit to + implementing it right now. + +* We will not fix or apply amnestly in order to re-enable stlylint or + coverage-js. That could take significant effort, which we believe would be + better spent completing the migration off of this legacy Sass and JS and onto + our modern React frontends. + +Soapbox +******* + +The ADR author would like to take this moment to make some highly-opinionated +assertions: + +* We need to do a better job validating that changes to our CI don't break our + CI. + +* Every layer that we have between CI and the code which is checking could be + hiding a bug that may cause false-positives. That is why we are ripping Paver + out of edx-platform CI. Going forward, we should be extremely skeptical of + tools or scripts which create abstraction between CI and application code. + +* We should perhaps emphasize the importance of shell scripting proficiency for + core contributors who will be working on tooling/CI. + + +.. _which we have been trying to move off of: https://github.com/openedx/edx-platform/issues/34467 +.. _announced on the forums: https://discuss.openedx.org/t/checking-pii-annotations-with-a-lower-coverage-threshold/14254 +.. _OEP-30: https://docs.openedx.org/projects/openedx-proposals/en/latest/architectural-decisions/oep-0030-arch-pii-markup-and-auditing.html +.. _fix the malformed annotations: https://github.com/openedx/edx-proctoring/issues/1241