Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use definite article for single file error case; improve functional test output #5905

Merged
merged 1 commit into from
Apr 30, 2021

Conversation

eloquence
Copy link
Member

@eloquence eloquence commented Apr 16, 2021

Status

Ready for review

Description of Changes

Testing

Error message behavior

On this PR's branch:

  1. Spin up Docker env
  2. Remove all files from disk in the container's /var/lib/securedrop/store directory
  3. Attempt to download single file by clicking its filename directly in the Journalist Interface
    • Observe that single file error message is displayed
  4. Attempt to download single file via the checkbox interface
    • Observe that single file error message is displayed
  5. Attempt to download multiple files
    • Observe that multiple file error message is displayed

Improved assertion output

  1. Break one of the tests like so:
diff --git a/securedrop/tests/functional/journalist_navigation_steps.py b/securedrop/tests/functional/journalist_navigation_steps.py
index 3f3813a7d..da8e6d825 100644
--- a/securedrop/tests/functional/journalist_navigation_steps.py
+++ b/securedrop/tests/functional/journalist_navigation_steps.py
@@ -1189,7 +1189,7 @@ class JournalistNavigationStepsMixin:
         if single_file:
             error_msg = (
                 "Your download failed because the file could not be found. An admin can find "
-                + "more information in the system and monitoring logs."
+                + "more information in the system and monitoring logs. XYZZY"
             )
         else:
             error_msg = (
  1. Run it via securedrop/bin/dev-shell bin/run-test -s -v tests/functional/test_journalist.py -k test_download_source_unread^

@eloquence
Copy link
Member Author

(CI's probably going to fail, will let it do its thing and then update affected tests)

@eloquence eloquence force-pushed the single-file-message branch 2 times, most recently from fef599c to e8e1df8 Compare April 17, 2021 00:01
@codecov-commenter
Copy link

codecov-commenter commented Apr 17, 2021

Codecov Report

Merging #5905 (e8e1df8) into develop (fedce7a) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #5905   +/-   ##
========================================
  Coverage    85.49%   85.49%           
========================================
  Files           53       53           
  Lines         3902     3902           
  Branches       484      484           
========================================
  Hits          3336     3336           
  Misses         455      455           
  Partials       111      111           
Impacted Files Coverage Δ
securedrop/journalist_app/col.py 82.19% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fedce7a...e8e1df8. Read the comment docs.

@eloquence eloquence marked this pull request as ready for review April 17, 2021 06:23
@eloquence eloquence requested a review from a team as a code owner April 17, 2021 06:23
rmol
rmol previously approved these changes Apr 21, 2021
Copy link
Contributor

@rmol rmol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we wanted to show the single file error message in the case where one file is selected and "Download Selected" is clicked, we could check the length of the list of things to delete here.

I'm approving as is, and will merge unless you want to try that. The test plan went fine:

  1. Spin up Docker env
  2. Remove all files from disk in the container's /var/lib/securedrop/store directory
  3. Attempt to download single file
    • Observe that single file error message is displayed
  4. Attempt to download multiple files
    • Observe that multiple file error message is displayed

@eloquence
Copy link
Member Author

That would be nicer, I'll take another look, thanks @rmol

@deeplow
Copy link
Contributor

deeplow commented Apr 26, 2021

Wow. It is super interesting how little one realizes as a translator the engineering effort & implications of what looked like a simple thing.

Thank you for addressing this so quickly and keep up the good work!!

@eloquence eloquence force-pushed the single-file-message branch from 0d47f55 to 7796a8e Compare April 29, 2021 20:32
@eloquence eloquence changed the title Use definite article for single file error case Use definite article for single file error case; improve functional test output Apr 29, 2021
@eloquence
Copy link
Member Author

eloquence commented Apr 29, 2021

Thanks for the suggestion @rmol. This is now done and the correct message should be shown regardless of download method.

As part of looking at functional test output, I got frustrated that the assert failures didn't include diffs, and added assert rewrite logic which should fix this at least for the functional test helpers.

staging-test-* and translation-tests have been balking at me, but I'm not sure yet if this is related to anything in this PR.

@eloquence
Copy link
Member Author

Looks like it was just flaky tests, as usual :/

Copy link
Contributor

@rmol rmol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Tested single/multiple downloads, and verified the pytest assert rewriting in support functions, which is a nice enhancement.

@rmol rmol merged commit e37c6c0 into develop Apr 30, 2021
@rmol rmol deleted the single-file-message branch April 30, 2021 14:08
@zenmonkeykstop zenmonkeykstop mentioned this pull request Jun 14, 2021
39 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants