Add accessibility sniffs to pageslayout tests #6745
Conversation
|
@saffronsnail, thanks for checking in about this. Since your changes here are still WIP, this is not a full review, but I'm happy to discuss your questions so far.
The page-layout tests report via pytest, which we've configured to save to
I like your approach here, since the new
|
skyvine
force-pushed
the
accessibility-tests
branch
from
b425c57
to
663bccd
Compare
|
Thanks for the feedback! I think the new version of the branch should be good to go, I've also updated the first comment to reflect the current status. One thing that I noticed when updating the function name is several comments along the lines of: The function name doesn't explicitly say that it's taking a screenshot any more. I don't think this is a big problem especially with the docstring update, but just wanted to call attention to it in case it is more concerning than I assume. Also, the summary includes the full path to the accessibility-info directory at the top as a helpful notice. I'm not sure if this is too helpful - this reveals the developer's username if the summary file becomes public. This file is only generated as a result of Finally, the circle-ci doc mentions that, "[w]hen you save test data using the |
|
Here's a copy of the summary output on my machine in case it is of interest: Accessibility Summaryjournalist-account_edit_hotp_secret.txt: journalist-account_new_two_factor_hotp.txt: journalist-account_new_two_factor_totp.txt: journalist-admin.txt: journalist-admin_add_user_hotp.txt: journalist-admin_add_user_totp.txt: journalist-admin_changes_logo_image.txt: journalist-admin_edit_hotp_secret.txt: journalist-admin_edit_totp_secret.txt: journalist-admin_index_no_documents.txt: journalist-admin_interface_index.txt: journalist-admin_new_user_two_factor_hotp.txt: journalist-admin_new_user_two_factor_totp.txt: journalist-admin_ossec_alert_button.txt: journalist-admin_system_config_page.txt: journalist-clicks_on_source_and_selects_documents.txt: journalist-code-fail_login.txt: journalist-code-fail_login_many.txt: journalist-code-fail_to_visit_admin.txt: journalist-col.txt: journalist-col_has_no_key.txt: journalist-col_javascript.txt: journalist-col_no_document.txt: journalist-composes_reply.txt: journalist-delete_all.txt: journalist-delete_all_confirmation.txt: journalist-delete_none.txt: journalist-delete_one.txt: journalist-delete_one_confirmation.txt: journalist-edit_account_admin.txt: journalist-edit_account_user.txt: journalist-index.txt: journalist-index_javascript.txt: journalist-index_no_documents.txt: journalist-index_with_text.txt: journalist-login.txt: source-checks_for_reply.txt: source-deletes_reply.txt: source-enter-codename-in-login.txt: source-generate.txt: source-index.txt: source-login.txt: source-logout_page.txt: source-lookup-shows-codename.txt: source-lookup.txt: source-next_submission_flashed_message.txt: source-notfound.txt: source-session_timeout.txt: source-submission_entered_text.txt: source-tor2web_warning.txt: source-use_tor_browser.txt: source-why_journalist_key.txt: |
There was a problem hiding this comment.
Very nice, @saffronsnail! I've tested both stages of this linting locally:
make test TESTFILES=securedrop/tests/functional/pageslayoutmake accessibility-summary
I do want to follow up on your original idea:
Presumably, a new make target similar to
validate-test-htmlwill be added, or this target will be updated.
How do you envision this linting being used for reporting in CI? You say that you don't expect make accessibility-summary to be used this way. But I'm not sure what the alternative is: until we've squashed the most-recurrent errors, displaying (never mind erroring on) the per-test output won't be useful. As you note re: CircleCI's store_test_results step, one option is to write a custom test-results/junit.xml with the per-test output, but we still won't want to consider these errors as failures yet.
|
Ah, there was a misunderstanding. I thought that |
|
As before, I can sqaush these commits once the complete changeset looks good. |
skyvine
force-pushed
the
accessibility-tests
branch
from
a0a56ae
to
8a740c1
Compare
|
Looking at the circleci output, I have a couple of questions. First, is it expected that there is only output for 5 files? I'm assuming that the layout tests are running on a subset of files to keep it more manageable while working towards validatability, since the Second, I don't see a way to go to the files containing the full output in circleci. I wrote the target that way with an implicit assumption that this is running locally (I haven't worked with devops tools very much 😅), so I'm thinking it might be better to have the make target print the full output after (or, possibly, instead of) the summary. Thoughts? |
|
Hey @cfm, I'm not sure if this needs more active work or not based on the way it looks in CircleCI. As previously implied, this aspect of the CL was not fully considered before. My main point of uncertainty is about the developer's workflow, in order to determine whether or not the current CircleCI output is sufficient. If developers typically run the test suite locally, and CircleCI is just used for communicating results between people, then I expect the current CL will work fine because the developer can examine the files locally. But if developers typically use CircleCI as the mechanism for running the test suite, then it is certainly not sufficient. In case it is not sufficient, I will mark this as "Work in Progress" again since more work needs to be done. My plan is to write out json files instead of plain text files, and invert the page/error relationship in reporting (eg, "this error occurs in these locations of these pages" instead of "this page contains these errors in these locations) in order to minimize the amount of text that y'all will have to go through. I think this will also make it clearer what amount of impact fixing each error will have, when there is a common cause. But we'll see how it looks when it's actually implemented. Please let me know what you think whenever you have a moment! |
|
Thanks for following up, @saffronsnail. From #6745 (comment):
Good question! This behavior isn't caused by anything in your code. The key lines are: securedrop/.circleci/config.yml Line 134 in 5177650 securedrop/.circleci/config.yml Line 150 in 5177650 Together, these mean that the For this reason, although some developers and contributors may see these results only in CI and not locally, your current implementation makes sense. While you could "write out json files instead of plain text files, and invert the page/error relationship in reporting (eg, "this error occurs in these locations of these pages" instead of "this page contains these errors in these locations)", in CI that summary will still be only per-runner rather than global. And that's fine. :-) Does this make sense? If you're happy with the current state of this branch, I'll plan to review it on Monday. |
|
That makes sense! And in that case, I think the branch is fine as-is. Whenever you have the resources to review the CL works for me, I just wanted to make sure I knew if there was anything actionable on my end. =) |
cfm
force-pushed
the
accessibility-tests
branch
from
8a740c1
to
f187230
Compare
There was a problem hiding this comment.
This looks great, @saffronsnail! CI is currently failing from your fork. That's definitely not your fault, since the actual tests are passing. I think it's a side effect of #6739; I'm looking into it.
Testing: Since CI splits test results, I've tested locally with TESTFILES=tests/functional/pageslayout make test && make accessibility-summary, which reported on the entire test suite.
Code review: You've designed this nicely, given (a) how complex our test suite is anyway and (b) injecting html_codesniffer during each functional test. I've given some thought to whether "sniff" is a confusing concept and term to introduce in addition to "lint" and "validate". I think it's fine: it's specific to html_codesniffer, and it is different than static linting and validation.
I've left requested two changes inline; please review them when you get a chance. Tomorrow I'll ask the rest of the team for feedback on the introduction of npm and html_codesniffer in these Dockerfiles and get back to you.
|
Thanks @cfm ! I implemented both of the recommended changes in a single new commit. Hopefully |
|
I uploaded a commit that technically uses The primary issue with running it in the Dockerfile is that, if I'm understanding Docker correctly, we don't actually have the repository available. In I could add the html-codesniffer package itself into the repository. The full If adding |
As a true (non-phony) Make target, "node_modules" will be refreshed via "npm ci" only when "package-lock.json" has changed. That makes it a one-time operation (like "make venv" and equivalents) for developers and steps around the fact that "package-lock.json" is not available[1] at the containers' build-time. [1]: freedomofpress#6745 (comment)
|
Thanks for this proof-of-concept, @saffronsnail, and for walking me through your thinking. I think we can solve this dilemma:
Good catch. In the main securedrop/securedrop/dockerfiles/focal/python3/Dockerfile Lines 18 to 20 in e2d8bbf We could do the same thing here, by embedding the desired
Our main concern isn't whether the files themselves are available outside the container; this is true of the Python virtual environment too. We just want to make sure that What do you think, @saffronsnail? @legoktm, if you have time, I'd appreciate your sign-off too. |
|
Sorry, I feel really silly now, I didn't realize that html_codesniffer has no dependencies! Locking is pretty useless then (small benefit in that it pins sha256 checksum but that's pretty negligible). Given that, I think your original |
Accessibility sniffs going to be added to this function, meaning that the old name is no longer complete. Specifying each thing separately would make the name unwieldy, so change it to a generic name and add a docstring explicating the things it does.
skyvine
force-pushed
the
accessibility-tests
branch
from
a650874
to
b42208e
Compare
|
Ok, if I understood everything correctly then I think the new state of the PR should be mergable (assuming CircleCI is happy). I went back to the version that used I did add a new line to the Dockerfiles: This line runs before When there are dependencies, the word |
This commit follows the structure of the current pageslayout tests by collecting info in the `save_screenshot_and_html` function, and saving the results into a new subdirectory under `tests/functional/pageslayout/accessibility-info`. Under this there is a layer of subdirectories for the locale, then a second layer of subdirectories to organize messages between errors that require human review and errors that are machine-verified. Additionally, this adds a new make target `make accessibility-summary` which will process all of these data files and display a summary. This is done rather than displaying the full output because there are currently ~50,000 lines of output stored in these files; if we displayed only errors then this would cut it down to 2,600 lines which is a significant reduction, but still a lot for terminal output. Note that many of the messages are duplicates across pages. For example, the blue buttons with white text are slightly below the contrast guideline, and there is a message about this problem on nearly every page. This commit also adds html_codesniffer as a dependency to the project. This is installed globally into the docker image because html_codesniffer itself requires no dependencies. See PR 6745 for the full discussion. Co-authored-by: Cory Francis Myers <cfm@panix.com>
skyvine
force-pushed
the
accessibility-tests
branch
from
b42208e
to
c43f6f5
Compare
cfm
changed the title
Status
Ready for review
Description of Changes
This commit follows the structure of the current pageslayout tests by collecting info in the
save_screenshot_and_htmlfunction, with the intent that it will later be consumed in the appropriate context. Presumably, a new make target similar tovalidate-test-htmlwill be added, or this target will be updated.Notes for Reviewers
As previously discussed with @cfm, this commit adds code in Dockerfiles to install software from from
npm(context). This toolchain is new to the project.Testing
How should the reviewer test this PR?
Write out any special testing steps here.
make test TESTFILES=tests/functional/pageslayoutwill exercise all of the changes to the test suite. A summary of the results can be seen withmake accessibility-summary, which will process raw output into a summary file and open it inless. Full output can be found attests/functional/pageslayout/accessibility-info/. The directory structure is similar to that oftests/functional/pageslayout/html, but after the locale there are 2 separate directories,errorsandreviews, with the corresponding output saved to files using thetest_nameparameter.The summary file reports when the sniff output differs by locale (this is only approximate but I believe it to be reasonably accurate, see the implementation of
summarize_accessibility_resultsfor details) . Currently, there are no differences by locale. I exercised this code path by manually editing the text files created in themake testinvocation to simulate differences. Just delete any line starting with MessageType in any file to see this in action.Deployment
N/A
Checklist
If you made changes to the server application code:
make lint) and tests (make test) pass in the development containerIf you made changes to
securedrop-admin:N/A
If you made changes to the system configuration:
N/A
If you added or removed a file deployed with the application:
N/A
If you made non-trivial code changes:
Test suite question is N/A, this is a change to the test suite itself.
Choose one of the following:
Based on the link to freedomofpress/securedrop-docs#245 added by @cfm, it looks like there does need to be a doc update here?
If you added or updated a production code dependency:
I previously listed this as N/A because the dependency is not "production" in the sense that it does not need to be deployed to users, is that understanding accurate?