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

make functional test cassettes work in any order #1138

Merged
merged 3 commits into from
Aug 19, 2020
Merged

Conversation

sssoleileraaa
Copy link
Contributor

Description

Makes cassette-generation reproducible, that is, allows us to run generation of cassettes for each functional test in any order given a base system setup.

  • Updated the README to include instructions on how to set up the base requirements/ system setup for generating new cassettes
  • Also updated functional tests so that the can be run in any order

Test Plan

  • verify that currently we are unable to regenerate cassettes for functional tests by deleting the cassettes directory on the main branch and then running make test-functional
  • verify that by following the README instructions in this PR that you are able to re-generate cassettes in any order (randomly) on this branch

@sssoleileraaa
Copy link
Contributor Author

sssoleileraaa commented Aug 12, 2020

This should be reviewed and merged into main, not update-current-user-info-during-session, because this change doesn't 100% unblock #1135 (it'll remain in draft mode until I finish work on it).

@sssoleileraaa sssoleileraaa force-pushed the fix-cassette-generation branch from e348e31 to 2e77a7c Compare August 12, 2020 17:27
@sssoleileraaa
Copy link
Contributor Author

(rebased on main)

@sssoleileraaa
Copy link
Contributor Author

Based on feedback from John, I'm missing a critical step in my README instructions that allowed me to generate cassettes without having to worry about TOTP generation and expiration. I applied the following patch to my dev server so that we could quickly record new cassettes without having to modify the hard-coded TOTP in tests/conftest.py:

diff --git a/securedrop/models.py b/securedrop/models.py
index dcd26bbaf..b5743411a 100644
--- a/securedrop/models.py
+++ b/securedrop/models.py
@@ -649,6 +649,7 @@ class Journalist(db.Model):
 
         try:
             user = Journalist.query.filter_by(username=username).one()
+            #return user
         except NoResultFound:
             raise InvalidUsernameException(
                 "invalid username '{}'".format(username))
(END)

Since our functional tests use the same TOTP code each time they are run anyway, I don't think we lose coverage here. One day if we want to add functional tests that don't rely on cassettes then I think it makes sense for those to make actual API requests against the server and test things like login throttling. So right now I think it makes sense to document how things work today so we can move forward with work on things like allowing journalist names to be updated in the client sync. It would be a step in the right direction to just document how to get cassette generation working (what the server setup requirements are etc.), so I'll update the README with the missing step and recommend that we open an issue to capture ideas on functional test improvments around TOTP expiration and cassette generation and additional test coverage. Would appreciate feedback on whether or not there is a better way to generate cassettes and if others think this should be done within this PR.

@sssoleileraaa sssoleileraaa force-pushed the fix-cassette-generation branch 2 times, most recently from 595cddc to 861fe1c Compare August 13, 2020 05:39
README.md Outdated Show resolved Hide resolved
rmol
rmol previously requested changes Aug 14, 2020
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.

👍 Following the new instructions, cassettes were successfully regenerated.

I made one ridiculous nitpick about a comment. Sorry.

tests/functional/test_send_reply.py Outdated Show resolved Hide resolved
@sssoleileraaa sssoleileraaa force-pushed the fix-cassette-generation branch from d44cd93 to 02ee8c0 Compare August 15, 2020 02:38
@sssoleileraaa sssoleileraaa requested a review from rmol August 15, 2020 02:40
@rmol rmol dismissed their stale review August 18, 2020 14:16

The code involved has been eliminated.

@sssoleileraaa
Copy link
Contributor Author

the change to no longer delete the source after sending the reply is a good change - what i was trying to avoid failing other tests that expected the last item of the conversation to be something other than a reply widget, but instead i updated those tests to no longer look at the last item in the conversation and instead look at the specified index from top down.

this is ready for another review

@sssoleileraaa sssoleileraaa force-pushed the fix-cassette-generation branch from bf94fa2 to e915a11 Compare August 18, 2020 23:09
@sssoleileraaa sssoleileraaa force-pushed the fix-cassette-generation branch from e915a11 to 9d69ff5 Compare August 18, 2020 23:34
@sssoleileraaa
Copy link
Contributor Author

one more change I just added is that the reply test now checks that the number of items in the conversation increased by 1 (to avoid any false positives where there was already a reply as the last widget on the test source)

the nice thing about these last changes is that we now only need to set up the server with two sources, each with one file and one message, and at this point i think someone can run through the test plan again for final review. (heading over to sdk-land to see how we can make generating cassettes there easier)

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.

:shipit:

@rmol rmol merged commit 992393d into main Aug 19, 2020
@rmol rmol deleted the fix-cassette-generation branch August 19, 2020 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants