Expand functional test coverage to prevent regressions

[eloquence]

(Converted from card by @redshiftzero)

We need to critically examine bugs in securedrop-client from the last ~month or so and add functional tests sufficient to cover them (to prevent regressions going forward).

[sssoleileraaa]

I think many regressions and bugs are around hard-to-see visual discrepancies. Since the machines can diff better than we humans can, I think we should prioritize tests that catch whether or not our code changes affect the look of the application. When we do expect the application to look differently, we can take new screenshots of the changes and add those to our PRs so that these tests can pass.

@kushaldas pointed out that this kind of test cannot be a functional test, rather we would need to use integration test (see related discussion about adding integration tests at the workstation level: freedomofpress/securedrop-workstation#524). This has something to do with needing to run on a fixed display size, but I not sure I understand. Also, we'll need to think about how we may need to require Qubes screenshots to make sure widgets look the same in screenshots, etc.

[eloquence]

Just noting that our first agreed upon goal for the 4/2-4/15 sprint is to inventory and prioritize areas where we want to increase coverage. Once we have agreement on that, we can see if we still have time to begin adding some high priority tests.

@kushaldas pointed out that this kind of test cannot be a functional test, rather we would need to use integration test

Is that indeed a requirement? I'd also like to understand that better. How feasible would it be to attempt to detect visual differences in CI, without Qubes, just using the Docker dev env? It would seem to require that we would update the expected state with each PR that changes the appearance of the client -- which could be an acceptable price to pay.

[sssoleileraaa]

Since I've seen some gui regressions in the past few months around alignment, spacing, wrapping, and other visual side effects that can be hard to notice during development, I think it would be valuable to have some snapshots to compare against. For instance, one snapshot of the client with a populated source list and conversation view can help us detect dozens of possible regressions. It would be reassuring to know that adding say a wordwrap parameter to SecureQLabel or adding an animation to the FileWidget doesn't result in alignement or issues with text being hidden or cut off, etc. For example, see this snapshot that Conor provided: https://github.com/freedomofpress/securedrop-client.

Right now we don't have coverage for the sizing of widgets and layout positioning, so let's add this to the list whether or not we try to tackle it with a few screenshots vs many more unit and functional tests.

Another place we don't have coverage is how long things should take. Like at what point do we say this is taking so long that it's a regression? This can be handled in functional tests by carefully selecting how long we wait for client actions or responses to requests.

[eloquence]

We'll only be able to spend limited time on functional tests during the 4/22-5/6 sprint given the SD 1.3.0 release, but we've agreed that if we spot regressions during Workstation/Client QA, we may opportunistically increase functional test coverage at that time, with approximately a 4 hour timebox.

[eloquence]

We didn't really get to client QA yet in the previous sprint, but the same commitment still applies -- @kushaldas will also add his recommendations for areas to prioritize to this ticket.

[kushaldas]

• show sync animation during the entire sync #1053 (UI animation, no tests, human eyes maybe)
• Populate active source conversations first #1042 maybe a screenshot test
• Messages/replies cut off when sent together with files #998 maybe a screenshot test
• Tooltips shown for most SecureQLabels with long strings #1015 We need test for this via the Qwidget and tooltip
• horizontal scrollbar on source list #973 this will need a fucntional test?
• Make the login modal, thus ensuring only one instance is possible at once. #978 maybe another test?
• source selected by default on startup #962 maybe a tes?
• [GUI] "Failed to send" shrinks speech bubble #953 screenshot test
• Gracefully handle lack of submission GPG private key #140 this is working in progress by john, we need a test?
• When mousing over stars in offline mode, everything gets starred #938 maybe a screenshot test
• When you start the client in offline mode unstarring still works #935 maybe a screenshot test
• reconnecting text gets cut off on minimized window #932 maybe a screenshot test

[sssoleileraaa]

Once #1082 is merged then we'll have new UI coverage where we didn't before. Also some of the issues from @kushaldas's list above already have coverage. That said, we should be testing any major functionality in functional tests and it's fine to have overlap.

Here's a little additional info on the links above:

• show sync animation during the entire sync #1053 - this was a change request to show the sync animation during the entire metadata sync period. right now we have coverage for making sure the icon is our animated gif on sync start until the metadata sync ends.
• Populate active source conversations first #1042 - making sure that a source with the most recent activity appears at the top of the list should be covered by unit tests. if it's not we should add them.
• Messages/replies cut off when sent together with files #998 - a combo of the tests added in Move CSS to CSS files and add new tests for stylesheets #1082 and a screenshot test would be the best way to catch that only some text is visible in a speech bubble
• Tooltips shown for most SecureQLabels with long strings #1015 - as of Move CSS to CSS files and add new tests for stylesheets #1082 we'll have tests for the max size of widgets, including the tooltip, which is all we need to prevent this regression
• horizontal scrollbar on source list #973 - as of Move CSS to CSS files and add new tests for stylesheets #1082 we'll have tests for the max and min size for widgets, including the conversation scrollarea. it would also be useful to have a unit test that checks that the horizontal bar is disabled.
• Make the login modal, thus ensuring only one instance is possible at once. #978 - this was a regression when we started calling show instead of exec in order to make the functional tests work i believe... looks like no regression test was added here: Make the login modal, thus ensuring only one instance is possible at once. #978 so I agree this should be tested!
• source selected by default on startup #962 - on line 731 we have a regression test for this: https://github.com/freedomofpress/securedrop-client/pull/981/files#diff-ccbb0db36351f2226aa9a8d20e943187R731
• [GUI] "Failed to send" shrinks speech bubble #953 - as of Move CSS to CSS files and add new tests for stylesheets #1082 we'll have tests for the max and min size for widgets, including the Speech bubble and speech bubble area widget
• Gracefully handle lack of submission GPG private key #140 - yes, this manual test plan procedure around missing gpg keys should be in a functional tests: Improve error handling of downloads, speech bubbles #1059
• When mousing over stars in offline mode, everything gets starred #938 - yup, looking at https://github.com/freedomofpress/securedrop-client/pull/941/files and it looks like a regression test was never added - we can add a test to make sure we don't star any other source in the source list other than that one we expect to be starred
• When you start the client in offline mode unstarring still works #935 - making sure we can't star when in offline mode is covered:

  securedrop-client/tests/gui/test_widgets.py

  Lines 1594 to 1622 in 409db11

  	def test_StarToggleButton_on_pressed_offline(mocker):
  	"""
  	Ensure toggle is disabled when offline.
  	"""
  	controller = mocker.MagicMock()
  	controller.is_authenticated = False
  	stb = StarToggleButton(controller, 'mock_uuid', True)
  	stb.click()
  	stb.controller.on_action_requiring_login.assert_called_once_with()
  	def test_StarToggleButton_on_pressed_offline_when_checked(mocker):
  	"""
  	Ensure correct star icon images are loaded for the disabled button.
  	"""
  	controller = mocker.MagicMock()
  	controller.is_authenticated = False
  	source = factory.Source(is_starred=True)
  	stb = StarToggleButton(controller, source.uuid, source.is_starred)
  	set_icon_fn = mocker.patch('securedrop_client.gui.SvgToggleButton.set_icon')
  	stb.on_authentication_changed(False)
  	assert stb.isCheckable() is False
  	set_icon_fn.assert_called_with(on='star_on.svg', off='star_on.svg')
  	stb.click()
  	stb.controller.on_action_requiring_login.assert_called_once_with()

• reconnecting text gets cut off on minimized window #932 - a combo of the tests added in Move CSS to CSS files and add new tests for stylesheets #1082 and a screenshot test to make sure all text is visible at the minimum size is a good idea

[sssoleileraaa]

Was just thinking today about how we might want to have an easy securedrop screenshot test backup to use whenever we need to make a new screenshot due to a UI update.

[sssoleileraaa]

Also adding to the list:

• We need a placeholder regression test to make sure long journalist_designations don't get cut off. This would be most easily done in a screenshot test. /cc @kushaldas

[eloquence]

Stale issue with no obvious remaining scope; closing.