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

Add ci job that regenerates cassettes and reruns functional tests #1140

Open
sssoleileraaa opened this issue Aug 13, 2020 · 3 comments
Open

Comments

@sssoleileraaa
Copy link
Contributor

Description

The idea here is to find out in CI if the client functional tests fail when cassettes are regenerated before replaying to detect outdated cassettes. If our new CI test fails then we would know that the cassettes need to be updated in the current PR. I think this is also an example of where we might want to have an exception for our one-commit per PR (a guideline we've discussed in order to make it easier to backport changes). Or we could just open a new PR with the updated cassettes. This is more of a detail that can be decided later.

There is existing code for the securedrop-sdk we can use as boilerplate: https://github.com/freedomofpress/securedrop-sdk/blob/main/.circleci/config.yml#L31

Challenges

  1. First we need to make our server-setup scriptable.

This means we should update create_dev_data.py to allow passing something like NUM_DOCS so that we can include document attachments when setting up test servers. For now, I think it's fine to use a default filename and file contents that the functional tests would expect, but eventually it would be nice to randomize file contents and names.

We also might want a way to specify strings for messages since functional tests check that messages are what we expect. This is something we don't need to worry about with replies since we can send replies straight from the client. Or, we could work with the way create_dev_data.py already works by adding a file in the client that matches https://github.com/freedomofpress/securedrop/blob/develop/securedrop/specialstrings.py (note: the order of the strings is a bit confusing now because if we supply NUM_SOURCES=5 to create_dev_data.py then the first 5 strings in that list will show up in reverse order in the source list) so that we can make our test assertions accordingly.

  1. The CI job would need to be able to patch the server so that we can skip the TOTP check before regenerating the cassettes.

This is the easiest way to avoid making our cassette-generation code take too long. The long-test way is to detect when we get an auth error and try regenerating a new TOTP value (using pytotp, for instance) and then try one more time to make the request. I'm leaning towards fast and scrappy, but am ready debate and persuasion! The nice thing about skipping auth validation on the server-side is that we don't have to keep hardcoding new TOTP values in our login tests and in conftest.py. Instead we would always use 123456. Another pro is that CI would be faster (by how much I'm not sure, maybe I'm overestimating this but as our functional tests grow this could potentially become a pain point).

@eloquence
Copy link
Member

The CI job would need to be able to patch the server so that we can skip the TOTP check

Might it be useful to also be able to pass this as an option to the dev env via a variable? There are many dev circumstances where we're not interested in testing TOTP logic and where it could be nice to just be able to bypass it with 123456.

@sssoleileraaa
Copy link
Contributor Author

We're pretty much set up to work on this issue now.

The CI job would need to be able to patch the server so that we can skip the TOTP check

Might it be useful to also be able to pass this as an option to the dev env via a variable? There are many dev circumstances where we're not interested in testing TOTP logic and where it could be nice to just be able to bypass it with 123456.

Yup, we can create and apply the small patch within the create_dev_data.py script.

  1. First we need to make our server-setup scriptable.

This means we should update create_dev_data.py to allow passing something like NUM_DOCS so that we can include document attachments when setting up test servers. For now, I think it's fine to use a default filename and file contents that the functional tests would expect, but eventually it would be nice to randomize file contents and names.

We now attach by default two memo.txt files with the text "This is an example of a plain text file upload." (see freedomofpress/securedrop#5580) per source. We also now default to creating 3 sources, marking one source as seen (all source content is seen), one source with some files and messages seen, and one source with no seen files or messages.

We may want to add a `NUM_DOCS option, but it's not blocking us on creating this ci job.

We also might want a way to specify strings for messages since functional tests check that messages are what we expect. This is something we don't need to worry about with replies since we can send replies straight from the client. Or, we could work with the way create_dev_data.py already works by adding a file in the client that matches https://github.com/freedomofpress/securedrop/blob/develop/securedrop/specialstrings.py (note: the order of the strings is a bit confusing now because if we supply NUM_SOURCES=5 to create_dev_data.py then the first 5 strings in that list will show up in reverse order in the source list) so that we can make our test assertions accordingly.

For now, we can work with what we have and pass NUM_SOURCES=5 make dev and only delete a source from the bottom or top of the list so that we know what the expected strings should be.

@eloquence
Copy link
Member

Discussed in backlog review today; still a very useful improvement, but likely not something we can work on in the near-term.

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

No branches or pull requests

2 participants