`app-tests`: validates HTML source saved from page-layout tests #6072

cfm · 2021-08-18T01:35:49Z

Status

Ready for review

Description of Changes

Closes #1897 by:

installing html5validator in the test-requirements used in CI pipelines;
providing and using in all page-layout tests a convenience function FunctionalTest._save_html() in the spirit of FunctionalTest._screenshot();
adding a Make target validate-test-html to validate the HTML source saved from such tests; and
executing this target (as informational-only for now) after tests have run in app-tests.

Discussion points

This is arguably a strict form of linting, but (per discussion in Lint HTML #1897) requires the outputs of higher-level tests, in this case page-layout tests. Given this infelicity, is there some other way we want to integrate this check into CI? For example, we could run it only as part of the nightly workflow.
If we do want this to be part of routine CI: Do we want to include this step in app-tests, further prolonging its runtime? One alternative would be to factor out page-layout tests to a separate job altogether, in parallel to app-tests, and include this validation step there.
html5validator requires a JDK, installed here as default-jdk. Is this an acceptable addition to the development/test container?

Testing

After running page-layout tests locally, make validate-test-html reports meaningful results (i.e., lots of errors :-).
app-tests passes (e.g.) despite lots of "informational" errors reported in the "Validate HTML" step.

Deployment

Development-only; no deployment considerations.

Checklist

If you added or updated a code dependency:

~~I have performed a diff review and pasted the contents to the packaging wiki~~
~~I would like someone else to do the diff review~~

test-requirements only.

…e-layout tests

kushaldas

The code and rest of the change looks good to me. Tested locally, can get many errors like below:

"file:/root/code/securedrop/securedrop/tests/pageslayout/html/ar/journalist-admin_ossec_alert_button.html":1.1-1.26: error: Start tag seen without seeing a doctype first. Expected "<!DOCTYPE html>".
"file:/root/code/securedrop/securedrop/tests/pageslayout/html/ar/journalist-admin_ossec_alert_button.html":36.5-36.37: error: Element "div" not allowed as child of element "label" in this context. (Suppressing further errors from this subtree.)
"file:/root/code/securedrop/securedrop/tests/pageslayout/html/ar/journalist-admin_ossec_alert_button.html":39.5-39.37: error: Element "div" not allowed as child of element "label" in this context. (Suppressing further errors from this subtree.)
"file:/root/code/securedrop/securedrop/tests/pageslayout/html/ar/journalist-admin_ossec_alert_button.html":42.5-42.40: error: Element "div" not allowed as child of element "label" in this context. (Suppressing further errors from this subtree.)
"file:/root/code/securedrop/securedrop/tests/pageslayout/html/ar/journalist-login.html":1.1-1.26: error: Start tag seen without seeing a doctype first. Expected "<!DOCTYPE html>".
"file:/root/code/securedrop/securedrop/tests/pageslayout/html/ar/journalist-login.html":30.5-30.37: error: Element "div" not allowed as child of element "label" in this context. (Suppressing further errors from this subtree.)
"file:/root/code/securedrop/securedrop/tests/pageslayout/html/ar/journalist-login.html":33.5-33.37: error: Element "div" not allowed as child of element "label" in this context. (Suppressing further errors from this subtree.)
"file:/root/code/securedrop/securedrop/tests/pageslayout/html/ar/journalist-login.html":36.5-36.40: error: Element "div" not allowed as child of element "label" in this context. (Suppressing further errors from this subtree.)
make: *** [Makefile:231: validate-test-html] Error 255

But, in CI I can not see the actual errors. I guess we want that too (as mentioned in the test plan).

@SaptakS please let me know what do you think.

kushaldas · 2021-08-18T09:59:06Z

securedrop/dockerfiles/focal/python3/Dockerfile

@@ -13,6 +13,8 @@ RUN apt-get update && apt-get install -y paxctl && \
                       python3-pip python3-all python3-venv virtualenv libpython3.8-dev libssl-dev \
                       gnupg2 ruby redis-server git xvfb curl wget \
                       gettext paxctl x11vnc enchant libffi-dev sqlite3 gettext sudo \
+                       # for html5validator
+                       default-jdk \


Finally we are an enterprise application :)

cfm · 2021-08-18T15:27:03Z

@kushaldas in #6072 (review):

But, in CI I can not see the actual errors. I guess we want that too (as mentioned in the test plan).

This threw me too! As CircleCI is currently splitting the tests across the three parallel runs, run 0 (at least on this execution) doesn't include any page-layout tests, so it doesn't save any HTML to validate. But runs 1 and 2 do.

I tried to anticipate this by having the validate-test-html target start by echoing "Validating HTML source saved from any page-layout tests run" (emphasis added). But I will think about whether there's a better way to document this behavior and/or highlight it in the CircleCI view.

…ted (per review feedback) thread: #6072 (review)

…ted (per review feedback) Otherwise, "app-tests" runs to which CircleCI has not assigned any page-layout tests will appear to include a validation step that returns no results. thread: #6072 (review)

cfm · 2021-08-18T19:42:17Z

#6072 (comment):

I tried to anticipate this by having the validate-test-html target start by echoing "Validating HTML source saved from any page-layout tests run" (emphasis added). But I will think about whether there's a better way to document this behavior and/or highlight it in the CircleCI view.

In 2ed9fce I've added an explicit count of how many *.html files have been saved from page-layout tests and will be validated.

kushaldas

Testing steps

After running page-layout tests locally, make validate-test-html reports meaningful results (i.e., lots of errors :-).
app-tests passes (e.g.) despite lots of "informational" errors reported in the "Validate HTML" step.

Tested locally, works as expected. html directory is also marked in .gitignore.

cfm added 4 commits August 17, 2021 17:57

FunctionalTest._save_html(): saves HTML source for validation

71cd4ef

validate-test-html: runs html5validator on HTML source saved from pag…

6839111

…e-layout tests

adds _save_html(X.html) for every _screenshot(X.png)

196656a

app-tests: includes "validate-test-html" as informational step

d9cc59c

cfm requested a review from a team as a code owner August 18, 2021 01:35

kushaldas self-assigned this Aug 18, 2021

kushaldas suggested changes Aug 18, 2021

View reviewed changes

cfm mentioned this pull request Aug 18, 2021

Validate html freedomofpress/securedrop.org#885

Merged

cfm added a commit that referenced this pull request Aug 18, 2021

counts how many page-layout tests have saved HTML source to be valida…

8a2a874

…ted (per review feedback) thread: #6072 (review)

counts how many page-layout tests have saved HTML source to be valida…

2ed9fce

…ted (per review feedback) Otherwise, "app-tests" runs to which CircleCI has not assigned any page-layout tests will appear to include a validation step that returns no results. thread: #6072 (review)

cfm force-pushed the 1897-validate-html branch from 8a2a874 to 2ed9fce Compare August 18, 2021 19:40

kushaldas approved these changes Aug 19, 2021

View reviewed changes

kushaldas merged commit 54d0e8b into develop Aug 19, 2021

kushaldas deleted the 1897-validate-html branch August 19, 2021 07:33

cfm mentioned this pull request Aug 25, 2021

remove "informational" status of HTML validation once HTML is expected to validate :-) #6078

Open

This was referenced Sep 22, 2021

app-page-layout-tests: follows consistent $DOCKER_BUILD_ARGUMENTS #6105

Merged

app-page-layout-tests failing with timeout #6104

Closed

cfm mentioned this pull request Jan 23, 2023

review options for accessibility linting (versus documentation) #6314

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`app-tests`: validates HTML source saved from page-layout tests #6072

`app-tests`: validates HTML source saved from page-layout tests #6072

cfm commented Aug 18, 2021

kushaldas left a comment •

edited

Loading

kushaldas Aug 18, 2021

cfm commented Aug 18, 2021

cfm commented Aug 18, 2021

kushaldas left a comment

app-tests: validates HTML source saved from page-layout tests #6072

app-tests: validates HTML source saved from page-layout tests #6072

Conversation

cfm commented Aug 18, 2021

Status

Description of Changes

Discussion points

Testing

Deployment

Checklist

If you added or updated a code dependency:

kushaldas left a comment • edited Loading

Choose a reason for hiding this comment

kushaldas Aug 18, 2021

Choose a reason for hiding this comment

cfm commented Aug 18, 2021

cfm commented Aug 18, 2021

kushaldas left a comment

Choose a reason for hiding this comment

Testing steps

`app-tests`: validates HTML source saved from page-layout tests #6072

`app-tests`: validates HTML source saved from page-layout tests #6072

kushaldas left a comment •

edited

Loading