Include various client related strings in test data #5224

kushaldas · 2020-05-04T06:12:14Z

Status

Ready for review

Description of Changes

Fixes freedomofpress/securedrop-client#1080

Adds many different test strings to test securedrop-client text formatting.

Testing

make dev should have 2 sources
NUM_SOURCES=10 make dev should create 10 sources
NUM_SOURCES=ALL make dev shoudl create 43 sources
Use the securedrop-client against the local server and look many strange looking texts :)

Deployment

Any special considerations for deployment? Consider both:

Upgrading existing production instances.
New installs.

Checklist

If you made changes to the server application code:

Linting (make lint) and tests (make test) pass in the development container

If you made changes to `securedrop-admin`:

Linting and tests (make -C admin test) pass in the admin development container

If you made changes to the system configuration:

Configuration tests pass

If you made non-trivial code changes:

I have written a test plan and validated it for this PR

If you made changes to documentation:

Doc linting (make docs-lint) passed locally

If you added or updated a code dependency:

Choose one of the following:

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

lgtm-com · 2020-05-04T06:19:52Z

This pull request introduces 1 alert when merging fd8a0f2 into ecf6255 - view on LGTM.com

new alerts:

1 for Implicit string concatenation in a list

lgtm-com · 2020-05-04T06:44:32Z

This pull request introduces 1 alert when merging 69c070f into ecf6255 - view on LGTM.com

new alerts:

1 for Implicit string concatenation in a list

lgtm-com · 2020-05-04T11:13:51Z

This pull request introduces 1 alert when merging 211a394 into ecf6255 - view on LGTM.com

new alerts:

1 for Implicit string concatenation in a list

sssoleileraaa

I like this CLIENT_TEST_DATA env var solution. Nice work!

sssoleileraaa · 2020-05-13T17:55:05Z

Do you want to add all the different test strings from the wiki?

Up to you. I think since this is not default, it would be great to add all the test strings from https://github.com/freedomofpress/securedrop-client/wiki/Message-Test-Data and also maybe a couple from https://github.com/minimaxir/big-list-of-naughty-strings/blob/master/blns.txt as you see fit.

kushaldas · 2020-05-14T09:03:35Z

Do you want to add all the different test strings from the wiki?

Up to you. I think since this is not default, it would be great to add all the test strings from https://github.com/freedomofpress/securedrop-client/wiki/Message-Test-Data and also maybe a couple from https://github.com/minimaxir/big-list-of-naughty-strings/blob/master/blns.txt as you see fit.

@creviera I updated with more text form blns.txt, creating at least 20 sources will be nicer to consume all strings properly.

lgtm-com · 2020-05-14T09:14:21Z

This pull request introduces 1 alert when merging 63f9eeb into 514b9e9 - view on LGTM.com

new alerts:

1 for Syntax error

sssoleileraaa · 2020-05-14T14:22:11Z

this lgtm - linter test fails though:

create-dev-data.py:91:85: E0001: invalid syntax (<unknown>, line 91) (syntax-error)

might want to move the special strings into a txt file?

kushaldas · 2020-06-09T13:25:58Z

Our future test strings are so good that they are breaking Python linting tools 🐍🕺 🐍

********** Module specialstrings
specialstrings.py:63:85: E0001: invalid syntax (<unknown>, line 63) (syntax-error)
************* Module create-dev-data
create-dev-data.py:18:0: E0001: Cannot import 'specialstrings' due to syntax error 'invalid syntax (<unknown>, line 63)' (syntax-error)

lgtm-com · 2020-06-09T16:03:06Z

This pull request introduces 1 alert when merging 9a75a52 into 309f17e - view on LGTM.com

new alerts:

1 for Implicit string concatenation in a list

sssoleileraaa · 2020-06-10T00:20:08Z

securedrop/create-dev-data.py

+
+        client_test = int(os.getenv('CLIENT_TEST_DATA', 0))
+        for i in range(1, client_test):
+            create_source_and_submissions(i, client_test, client_test_data=True)


so you're passing client_test in for source_count? could you instead pass num_sources and make CLIENT_TEST_DATA a boolean that says whether or not special strings should be used?

UPDATE
Let's talk more tomorrow about the design and how we imagine QA will use this feature as well as how we might use this for our screenshot tests. It'll also make it easier for me to review.

Here's a version with Allie's suggested change above, which works nicely for me (feel free not to use this):

diff --git a/securedrop/create-dev-data.py b/securedrop/create-dev-data.py index e689c6080..de5b8ea85 100755 --- a/securedrop/create-dev-data.py +++ b/securedrop/create-dev-data.py @@ -59,18 +59,17 @@ def main(staging=False): # Add test sources and submissions num_sources = int(os.getenv('NUM_SOURCES', 2)) + client_test = bool(os.getenv('CLIENT_TEST_DATA', 0)) for i in range(1, num_sources + 1): if i == 1: # For the first source, the journalist who replied will be deleted create_source_and_submissions( - i, num_sources, journalist_who_replied=journalist_tobe_deleted + i, num_sources, journalist_who_replied=journalist_tobe_deleted, + client_test_data=client_test ) continue - create_source_and_submissions(i, num_sources) + create_source_and_submissions(i, num_sources, client_test_data=client_test) - client_test = int(os.getenv('CLIENT_TEST_DATA', 0)) - for i in range(1, client_test): - create_source_and_submissions(i, client_test, client_test_data=True) # Now let us delete one journalist db.session.delete(journalist_tobe_deleted) db.session.commit()

further thoughts in here: #5224 (review)

sssoleileraaa

After my conversation with Ro about how this feature could be used during QA, I think it would make sense to have a follow-up discussion about a) populating production and staging servers with the same test data that we use here in our development environment so it can be used during QA, and b) what our default configuration of the server should be when testing.

Even though the change in this PR only applies to the dev container, I tried to think about how this would be used by QA in the following feedback:

Like messages, we should be able to populate replies with all the different test data
The default configuration should be the most broadly useful during test. For me, this means it should be fast so that I can test as I develop (this is less important for QA testers but it doesn't hurt), as well as easily reproducible to faciliate creating STRs when bugs are found.

So far your change meets this criteria but I think we can take it a step further and make it the default, i.e. running make dev should use all the test data strings. This would mean that you could get rid of CLIENT_TEST_DATA all together and skip converting it into a Boolean. So, by default, you could just populate the client with as many sources as you need in order to show all strings in the test data file, and if NUM_SOURCES is set to more or less than that, you just cycle through the list. It would be convenient to only show one message and one reply by default that each show the same test data string.
In the future it would be nice to also add to the default configuration special cases, such as:
- a source with no messages or replies
- a source with one message
- a source with one message and file
- a source with one file
- a source with one reply
In the future we could add a scalability-testing envrionment variable for configuring sources with many, many messages and replies as well.

lgtm-com · 2020-06-11T13:39:09Z

This pull request introduces 1 alert when merging 2fca9dd into 2c65008 - view on LGTM.com

new alerts:

1 for Implicit string concatenation in a list

rocodes · 2020-06-11T14:55:53Z

@kushaldas Tried the changes in 2fca9dd and imo they look good and work well. I'm not going to weigh in on whether make dev should make automatically with this number of sources (adds maybe 10 seconds to make time?), so I'll leave others to weigh in on the right amount of test data to be the default, but I think this looks good.

A few other points that came out of my conversation with @creviera:

There could be a useful integration between this and qa_loader down the line
The idea of reproducible data being populated during make dev paves the way nicely for more automated testing/screenshots in future too.

eloquence · 2020-06-11T16:35:26Z

Personally I like the behavior as it is now; for new developers using the development environment, it doesn't add a huge performance penalty (creation of sources is pretty zippy; it takes 15 seconds to create 43 sources on my older X1), and for more experienced devs, it's easy enough to add NUM_SOURCES. I think in general there's some value in doing more testing with a more highly populated environment by default.

eloquence · 2020-06-11T16:47:42Z

The one caveat is that all replies are identical right now. We've sometimes seen issues that arise only in messages, or only in replies, such as the recent styling differences reported in freedomofpress/securedrop-client#1082 (comment), or the context menu bug in freedomofpress/securedrop-client#1064 . I don't think this absolutely needs to be resolved in this PR, but it would be IMO preferable for messages and replies to both use the special strings file.

sssoleileraaa · 2020-06-11T17:26:00Z

Yeah, I agree that we can build on top of this change and that it doesn't need to do all the things. To be clear about what my review comment is requesting for changes:

Like messages, we should be able to populate replies with all the different test data

This is to catch bugs that only occur in the reply bubbles. Reply bubbles descend from the same class as message bubbles but they introduce new GUI behavior, like adding failure messages and changing the color bar, and so can we might miss a styling bug if we only show test data strings in message bubbles.

Also, this would allow us to see special strings in preview snippets, which would allow us to catch more bugs.

This can be made into a smaller issue that can be done outside of this PR.

The default configuration should be the most broadly useful during test. For me, this means it should be fast so that I can test as I develop (this is less important for QA testers but it doesn't hurt), as well as easily reproducible to faciliate creating STRs when bugs are found.

So far your change meets this criteria but I think we can take it a step further and make it the default, i.e. running make dev should use all the test data strings.

As Ro pointed out this would add a lot of time when starting make dev. The only problem I see with not doing this is that we will have to know the magic number to pass to NUM_SOURCES in order to see all the test strings.

This doesn't need to hold up your PR, but it is confusing as is, because when you run NUM_SOURCES=1 CLIENT_TEST_DATA=10 make dev you get 10 sources with 2 different special strings each in 2 different messages.

eloquence · 2020-06-11T17:28:00Z

The only problem I see with not doing this is that we will have to know the magic number to pass to NUM_SOURCES in order to see all the test strings.

I really don't think that's desirable - testers should not have to count up the number of test cases or look for a magic number somewhere. If we don't want to default to importing all, IMO there should be a symbolic way to specify this, e.g., NUM_SOURCES=ALL.

sssoleileraaa · 2020-06-11T17:34:59Z

Personally, I still prefer:

make dev - sets up a test server with all the strings in the test data file
NUM_SOURCES=1 make dev - sets up a test server with 1 source that has the special strings from the top of the test data file list
NUM_SOURCES=10 make dev - sets up a test server with 10 sources that have the special strings from the top of the test data file list (might not cycle through all the strings)
NUM_SOURCES=1000 make dev - sets up a test server with 1000 sources that have the special strings from the test data file list (cycles through all the strings multiple times)

And in the future... maybe something like:

NUM_MESSAGES=200 NUM_SOURCES=10 make dev - sets up a test server with 10 sources that each have 200 messages+replies that have special strings from the the test data file list (cycles through all the strings multiple times)

sssoleileraaa · 2020-06-11T17:39:08Z

Agreed, if we want to make make dev default to a smaller number of sources to speed things up, then NUM_SOURCES=ALL make dev is a nice solution.

lgtm-com · 2020-07-06T20:59:13Z

This pull request introduces 1 alert when merging 46a5d87 into beced1f - view on LGTM.com

new alerts:

1 for Implicit string concatenation in a list

sssoleileraaa · 2020-07-07T00:59:31Z

I think this needs to be kicked twice due to the test timeout issue.

sssoleileraaa · 2020-07-07T01:00:50Z

(referring to Too long with no output (exceeded 35m0s): context deadline exceeded)

eloquence · 2020-07-07T22:29:25Z

Taking a quick look at 46a5d87 ...

securedrop/create-dev-data.py

lgtm-com · 2020-07-07T22:59:23Z

This pull request introduces 1 alert when merging c83d8c4 into ebc193c - view on LGTM.com

new alerts:

1 for Implicit string concatenation in a list

eloquence · 2020-07-07T23:12:44Z

LGTM.

Test plan:

make dev should have 2 sources
NUM_SOURCES=10 make dev should create 10 sources
❌ NUM_SOURCES=ALL make dev should create 43 sources -- test plan is inaccurate
NUM_SOURCES=ALL make dev creates the required number of sources to ingest all strings

Recent changes by Allie LGTM. May benefit from a squash before merge.

lgtm-com · 2020-07-07T23:13:59Z

This pull request introduces 1 alert when merging d370fb0 into ebc193c - view on LGTM.com

new alerts:

1 for Implicit string concatenation in a list

conorsch

OK to merge once CI passes. Thank you @creviera for your patient review on this one!

One can get all the client source strings by using the `NUM_SOURCES=ALL make dev` command. The `make dev` command will create 2 sources as usual. One can also assign the number of sources (say 10) by `NUM_SOURCES=10 make dev` command. Fixes #1080 Special strings sourced from: https://github.com/freedomofpress/securedrop-client/wiki/Message-Test-Data

eloquence · 2020-07-07T23:25:43Z

(Squashed)

conorsch

Re-approving post-squash.

lgtm-com · 2020-07-07T23:32:10Z

This pull request introduces 1 alert when merging 9d82324 into ebc193c - view on LGTM.com

new alerts:

1 for Implicit string concatenation in a list

eloquence · 2020-07-07T23:37:42Z

Erm, that LGTM alert actually looks legit, looks like we're concatenating strings that should be separate per https://github.com/freedomofpress/securedrop-client/wiki/Message-Test-Data

conorsch · 2020-07-07T23:42:06Z

@eloquence Nice catch, I agree, we should fix that prior to merge. Thanks, LGTM: https://lgtm.com/projects/g/freedomofpress/securedrop/rev/pr-7c7409194c43f3b116ae35ee0fa18f076d1125dfh

conorsch

LGTLGTM, LGTM.

kushaldas force-pushed the more_fancy_client_test_strings branch from fd8a0f2 to 69c070f Compare May 4, 2020 06:37

kushaldas force-pushed the more_fancy_client_test_strings branch from 69c070f to 211a394 Compare May 4, 2020 11:06

eloquence changed the title ~~Fixes #1080 Various client related strings from test data~~ Include various client related strings in test data May 4, 2020

sssoleileraaa previously approved these changes May 13, 2020

View reviewed changes

kushaldas dismissed sssoleileraaa’s stale review via 63f9eeb May 14, 2020 08:56

kushaldas force-pushed the more_fancy_client_test_strings branch from 63f9eeb to 9a75a52 Compare June 9, 2020 15:52

kushaldas marked this pull request as ready for review June 9, 2020 16:22

kushaldas requested a review from redshiftzero as a code owner June 9, 2020 16:22

sssoleileraaa reviewed Jun 10, 2020

View reviewed changes

sssoleileraaa suggested changes Jun 10, 2020

View reviewed changes

kushaldas force-pushed the more_fancy_client_test_strings branch from 9a75a52 to 2fca9dd Compare June 11, 2020 13:31

eloquence reviewed Jul 7, 2020

View reviewed changes

securedrop/create-dev-data.py Outdated Show resolved Hide resolved

sssoleileraaa force-pushed the more_fancy_client_test_strings branch from 602e438 to c83d8c4 Compare July 7, 2020 22:49

conorsch mentioned this pull request Jul 7, 2020

CI: staging-test-with-rebase timeout at "Perform safe upgrade" stage #4901

Open

conorsch previously approved these changes Jul 7, 2020

View reviewed changes

kushaldas and others added 2 commits July 7, 2020 16:21

Add special string replies; remove magic number

9d82324

eloquence dismissed conorsch’s stale review via 9d82324 July 7, 2020 23:24

eloquence force-pushed the more_fancy_client_test_strings branch from d370fb0 to 9d82324 Compare July 7, 2020 23:24

conorsch self-requested a review July 7, 2020 23:25

conorsch previously approved these changes Jul 7, 2020

View reviewed changes

conorsch self-requested a review July 7, 2020 23:42

Fix missing comma

d4af328

eloquence dismissed conorsch’s stale review via d4af328 July 7, 2020 23:45

conorsch approved these changes Jul 9, 2020

View reviewed changes

conorsch merged commit 6f36b56 into develop Jul 9, 2020

eloquence mentioned this pull request Aug 3, 2020

Update create_dev_data.py to use test input strings freedomofpress/securedrop-client#1080

Closed

sssoleileraaa deleted the more_fancy_client_test_strings branch October 15, 2020 22:04

Include various client related strings in test data #5224

Include various client related strings in test data #5224

Conversation

kushaldas commented May 4, 2020 • edited by eloquence Loading

Status

Description of Changes

Testing

Deployment

Checklist

If you made changes to the server application code:

If you made changes to securedrop-admin:

If you made changes to the system configuration:

If you made non-trivial code changes:

If you made changes to documentation:

If you added or updated a code dependency:

lgtm-com bot commented May 4, 2020

lgtm-com bot commented May 4, 2020

lgtm-com bot commented May 4, 2020

sssoleileraaa left a comment

Choose a reason for hiding this comment

sssoleileraaa commented May 13, 2020

kushaldas commented May 14, 2020

lgtm-com bot commented May 14, 2020

sssoleileraaa commented May 14, 2020

kushaldas commented Jun 9, 2020

lgtm-com bot commented Jun 9, 2020

sssoleileraaa Jun 10, 2020 • edited Loading

Choose a reason for hiding this comment

redshiftzero Jun 10, 2020 • edited Loading

Choose a reason for hiding this comment

sssoleileraaa Jun 10, 2020

Choose a reason for hiding this comment

sssoleileraaa left a comment • edited Loading

Choose a reason for hiding this comment

lgtm-com bot commented Jun 11, 2020

rocodes commented Jun 11, 2020

eloquence commented Jun 11, 2020

eloquence commented Jun 11, 2020

sssoleileraaa commented Jun 11, 2020

eloquence commented Jun 11, 2020

sssoleileraaa commented Jun 11, 2020

sssoleileraaa commented Jun 11, 2020

lgtm-com bot commented Jul 6, 2020

sssoleileraaa commented Jul 7, 2020

sssoleileraaa commented Jul 7, 2020

eloquence commented Jul 7, 2020

lgtm-com bot commented Jul 7, 2020

eloquence commented Jul 7, 2020 • edited Loading

lgtm-com bot commented Jul 7, 2020

conorsch left a comment

Choose a reason for hiding this comment

eloquence commented Jul 7, 2020

conorsch left a comment

Choose a reason for hiding this comment

lgtm-com bot commented Jul 7, 2020

eloquence commented Jul 7, 2020

conorsch commented Jul 7, 2020

conorsch left a comment

Choose a reason for hiding this comment

kushaldas commented May 4, 2020 •

edited by eloquence

Loading

If you made changes to `securedrop-admin`:

sssoleileraaa Jun 10, 2020 •

edited

Loading

redshiftzero Jun 10, 2020 •

edited

Loading

sssoleileraaa left a comment •

edited

Loading

eloquence commented Jul 7, 2020 •

edited

Loading