docs: ADR for Fixing Quality and JS Checks

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