Tests against a local instance in the container #3628
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kushaldas
requested review from
heartsucker,
msheiny and
redshiftzero
as code owners
|
@redshiftzero @msheiny The admin test now creates real users and test those, so it takes more than 10 minutes in total. How to change the CI so that it does not fail for that? |
|
Ran the entire functional test suite via: Astoundingly, only two errors reported! I'll try with subsets of the tests and try to isolate the failure. The failure output is terribly verbose, and I forgot to pipe to |
|
In the interest of transparency, I'll keep a running list of which function test files have passed for me, so others can compare the behavior:
|
|
Updated test report with what's failing and what's not. @kushaldas please have a look and let me know if your experience has been any different. Haven't investigated the cause of the failures yet—clearly, the warnings on source interface tests will fail due to actual use of Tor Browser 😄—but we should be able to resolve prior to merge. Reading through the commit history, we should definitely perform some squashing to clarify why changes were made to which files. We'll likely have to do the same when merging As it stands, there's little to no visible justification for the use of a fork for the |
conorsch
reviewed
Jul 13, 2018
securedrop/Dockerfile
Outdated
| RUN chown -R $USER_NAME.$USER_NAME /tmp/.local/ | ||
|
|
||
| RUN wget -O /opt/torsocks_2.2.0-2_amd64.deb http://mirrors.kernel.org/ubuntu/pool/universe/t/torsocks/torsocks_2.2.0-2_amd64.deb | ||
| RUN dpkg -i /opt/torsocks_2.2.0-2_amd64.deb && apt-get install -f |
There was a problem hiding this comment.
The strategy for installing a more recent version of torsocks fetches the package over HTTP and installs whatever it gets, with no checksum or signature verification beforehand. That's bad. Perhaps we can sidestep the need to fetch a newer version of torsocks entirely if we instead use nc to pass the connection off to tor. Whatever version of nc is in the Trusty repos should be sufficient to pull that off!
There was a problem hiding this comment.
@conorsch Can you please help me with that patch? In my mind I would prefer to rebuild and store a version of torsocks somewhere and then pull and use that.
We should try with the latest upstream release and see if that solves our problem. I am also with the idea that maintaining our fork is always difficult. |
This is due to the test for orbot and warning. |
|
I pushed a commit which should fix our |
| @@ -24,6 +24,7 @@ mkdir -p "/tmp/test-results/logs" | |||
|
|
|||
| : "${PAGE_LAYOUT_LOCALES:=en_US,ar,fr_FR}" | |||
| export PAGE_LAYOUT_LOCALES | |||
| export TOR_FORCE_NET_CONFIG=0 | |||
There was a problem hiding this comment.
Let's document why this was used in a commit message
There was a problem hiding this comment.
Nevermind I see you did this! ❤️ :-)
| self.gpg = env.init_gpg() | ||
| db.create_all() | ||
|
|
||
| # Add our test user |
There was a problem hiding this comment.
Can we re-use create-dev-data.py for this?
There was a problem hiding this comment.
I tried, but, could not import from there, that is why added here.
| @@ -4,6 +4,7 @@ | |||
| # | |||
| # pip-compile --output-file securedrop/requirements/securedrop-app-code-requirements.txt securedrop/requirements/securedrop-app-code-requirements.in | |||
| # | |||
| -e git+https://github.com/freedomofpress/python-gnupg.git@af4508260d6eaa2b05efc59f8253585b7be29ad3#egg=gnupg | |||
There was a problem hiding this comment.
@kushaldas @msheiny: Regarding this psutil issue you both ran into on import of python-gnupg, can you confirm that the issue was due to a zombie process? That is, was the exception raised psutil.ZombieProcess?
There was a problem hiding this comment.
And if yes, my next question is: why are there so many gpg-agent zombie processes on the system? Was this seen in local machine, in the dev container, CI or some combination of the above?
Making a temporary fork simply for debugging makes a lot of sense 👍. That said, we shouldn't merge the Tor Browser functional testing feature branch with the fork of python-gnupg. Generally, we'll avoid creating a fork of libraries unless it becomes absolutely necessary.
There was a problem hiding this comment.
I did not see any zombie processes. It was trying to access PID 2.
There was a problem hiding this comment.
Ah, hm, what was the psutil exception? NoSuchProcess?
There was a problem hiding this comment.
nah @redshiftzero -- im pretty certain it was psutil.AccessDenied
There was a problem hiding this comment.
OK, thanks @msheiny. I was not able to reproduce this issue on macOS on Friday (well, I can artificially generate an exception by putting a breakpoint in the GPGMeta. _find_agent method and killing the process, which produces psutil.NoSuchProcess, but that's not really a reproducer).
The reason I was asking is to understand if the broad exception handling over in isislovecruft/python-gnupg#203 was necessary (or if a different approach would be better), but it seems like the original PR is the right call given that you hit psutil.AccessDenied, and the user in the original issue on FreeBSD (isislovecruft/python-gnupg#201) that was having this issue hit psutil.ZombieProcess
There was a problem hiding this comment.
Hi @redshiftzero! Thanks for alerting me about this issue via email! I get a lot of github notifications and I can't address all of them. Sorry for the delay!
I've merged that PR and a couple more fixes. If there's anything else SecureDrop needs, feel free to ping me. I have a bit of time in the next couple days to fix bugs and/or review patches.
There was a problem hiding this comment.
The upstream PR has been merged: isislovecruft/python-gnupg#203 so we're in a better position now to include the new logic, rather than maintaining a fork. My initial understanding of the introduction of this new dependency was that it was required to get the TBB logic working; during sprint planning today, however, @kushaldas indicated that the problem is not related to the TBB work, and can sometimes occur on develop.
One possibility is that the issue only arises on grsecurity-patched workstations, given that CI seems relatively happy, I'm able to run make test on my workstation on the develop branch without error, on a grsecurity-patched workstation, so I'm inclined to say that's not the case. Either way, I'd like to remove this change from the current PR and defer to a separate ticket, so reduce the scope of work we're reviewing and queuing up for a future release.
There was a problem hiding this comment.
Pleased to report that backing out the gnupg dependency change locally did not break the functional tests. Note that two separate commits had to be reverted to back out the change entirely. It may be that the change was introduced to fix issues with non-functional tests, but I couldn't find any record of that. I'll push up the changes I've made locally to this branch, then continue with the general cleanup and rebase tomorrow.
There was a problem hiding this comment.
awesome, thanks @isislovecruft!
securedrop/bin/run-test
Outdated
| @@ -5,7 +5,7 @@ set -euo pipefail | |||
|
|
|||
| source "${BASH_SOURCE%/*}/dev-deps" | |||
|
|
|||
| run_xvfb & | |||
| #run_xvfb & | |||
There was a problem hiding this comment.
Hm.. why is this commented out? I didn't see anything in the commit messages about this, apologies if I missed it
There was a problem hiding this comment.
Ah, this is my mistake. I will clean things out in a commit based on the review.
There was a problem hiding this comment.
Looks like the the removal of run_xvfb has not yet been addressed; I will attempt patching locally and fix if tests still pass.
There was a problem hiding this comment.
Can confirm that re-enabling run_xvfb locally does not break functional tests, so will include that modification in the upcoming cleanup.
|
I'm running into trouble testing this locally, as in all functional tests (that are not xfailed) are failing for me... is a testing step missing? I am:
Running functional test files one at a time also produces the same behavior. Full output |
|
@redshiftzero Technically I messed with my virtualenv a bit while reviewing the various TBB-related branches recently. I don't expect it created a divergence, but let me purge my venv and recreate, to see if I can reproduce your trouble. The description you provide is somewhat similar to the problems I had in #3488 (comment). Will report back on results of fresh venv. |
msheiny
reviewed
Jul 16, 2018
|
|
||
|
|
||
| Update this information to the `functional/instance_infomration.json file. | ||
| Update this information to the `tests/functional/instance_infomration.json file. |
There was a problem hiding this comment.
nit typo :) s/instance_infomration.json/instance_information.json/
There was a problem hiding this comment.
The instance_information.json typo is still present in this branch, and in fact preventing me from located the docs when I first tried to test here. Will include in upcoming cleanup commits.
| @@ -4,6 +4,7 @@ | |||
| # | |||
| # pip-compile --output-file securedrop/requirements/securedrop-app-code-requirements.txt securedrop/requirements/securedrop-app-code-requirements.in | |||
| # | |||
| -e git+https://github.com/freedomofpress/python-gnupg.git@af4508260d6eaa2b05efc59f8253585b7be29ad3#egg=gnupg | |||
There was a problem hiding this comment.
nah @redshiftzero -- im pretty certain it was psutil.AccessDenied
@redshiftzero After cloning fresh from @kushaldas's fork and creating a new virtualenv, I am able to run the entire functional test suite with only a single failure. As @kushaldas noted in standup, perhaps you have a dangling |
|
In my latest run: |
|
Will update the branch based on the review comments tomorrow morning my time. |
|
yep |
redshiftzero
force-pushed
the
local_tests
branch
from
640e8dc to
067c57f
Compare
|
I've:
|
|
Note that to close #3483, functional tests must pass for:
It's unclear what is going on with the test failures that are still occurring in CI after rebase. I do not see those failures locally. Investigating.. |
|
|
||
| # Set window size and position explicitly to avoid potential bugs due | ||
| # to discrepancies between environments. | ||
| #self.driver.set_window_position(0, 0) |
There was a problem hiding this comment.
Is it necessary to remove this? Standardizing this prevents us from having divergent window sizes between CI and dev that can produce CI test failures that are annoying to debug
There was a problem hiding this comment.
We can get these back into the code.
There was a problem hiding this comment.
I finally figured out why i had to comment that out. mozilla/geckodriver#676 is causing the trouble as we have to test against the firefox 52esr in this branch. None of those position calls are working in this. @redshiftzero
| @@ -8,7 +8,6 @@ | |||
| # We'll catch that error and respond accordingly in the next task. | |||
| failed_when: false | |||
| register: paxctl_firefox_header_check | |||
| when: ansible_kernel.endswith('-grsec') | |||
There was a problem hiding this comment.
Why is it necessary to remove this? (while the app-test role is used in staging, which does use grsec kernels, it doesn't hurt to leave this in explicitly no? Or did this produce some other issue?)
There was a problem hiding this comment.
Ahh I see. For other reviewers, the when condition was duplicated (see below)
|
Can now report that all functional tests are passing locally. We're still having CI woes, which I propose we handle separately. Merging this PR as-is will not break develop, as it's target a long-lived feature branch. For the next sprint, we should drill down on the CI problems to resolve the container logic, as we've done with the other test suites already. It's also worth noting that there are a number of debugging/WIP commits in this branch. Those should absolutely be cleaned up, i.e. via interactive rebase, prior to landing in develop. As we're closing out a sprint right now, I suggest we merge as-is, and focus on the rebase in the So, next steps required are:
|
|
I just figured that #1502 will also get closed when we merge this. |
|
Most of the tests passed for me, but I got one "Broken pipe" exception and a couple of test failures like this: Full output |
|
That's good news, @eloquence ! The |
|
@conorsch Sorry for my language problem, I wanted to remove Means the commit |
The 'when' conditional detecting a grsec kernel, used for running the paxctl commands on the TBB binary, was needlessly duplicated on the relevant task. Fortunately that didn't cause breakage, because the 'when' lines were identical, but only one was active.
Now installs Firefox 52 ESR, rather than Firefox 46, for use inside the
test container.
Includes changes to run-test shell script:
TOR_FORCE_NET_CONFIG=0 is required to directly connect to Tor
network, otherwise it will wait for userinput to either connect
or to configure
The `run_xvfb` invocation is no longer necessary, since the test suite
code bootstraps the headless server now.
|
@conorsch All functional tests passed locally. 💯 |
|
Same! Down to 20, rather than 21, given your latest commit. I'm going to squash your latest into the history, then we're set for merge here. |
Bootstrapping the application services within the functional test suite. Includes some cleanup, culling unused debugging code, and also cleans up the various print statements. Ignore functional test firefox logs (thanks, @msheiny!) Adds retries for tor network connection failure, using the pre-existing logic.
The version of torsocks in the Trusty repos isn't recent enough to support custom ports. Rather than install from other sources, which requires manual package verification (or configuring non-trusty repos, which could break other packages), let's fall back to good ol' nc.
We need to create a new firefox profile to test the orbot specific warning. This works for both locally and over Tor.
Now we can safely execute the account changes in the tests running on the Tor browser. The logic update makes sure to create different user for this test than any other test.
We don't have to sleep for too long if we are running against local instance. The ultimate goal remains to remove hardcoded sleeps altogether, but we'll circle back and eliminate those calls once the test suite is passing reliably.
The functional tests can take a long time, so let's instruct CircleCI to continue waiting, to give the test suite a chance to finish successfully.
This commit represented a squash of 3, all written by @msheiny, during a debugging session trying to get CI to pass on the long-lived TBB Selenium feature branch. CI isn't yet passing, so we'll punt to a separate ticket and revisit prior to merge into develop. Preserving the original commit messages below. 1. CircleCI - attempt to standardize CI to match devs Not sure this will solve the issue but occasionally we chase phantom bugs that only exist in CI and are really annoying to reproduce locally. Lets try to use the machine builder in CI instead to solve this problem. 2. Python3 not available in machine executor Lets just default to whatever is in python path.. code is compatible with both py 2.x and 3.x 3. Fix caching permissions issues for machine executor
Mostly correcting a typo in the `instance_information.json` config filename, but also updated some of the example commands. The notes regarding potentially failing tests also seemed out of date, as several members of the team have confirmed working functional tests under the new TB Selenium logic recently.
conorsch
approved these changes
Jul 26, 2018
This was referenced Jul 26, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Status
Ready for review.
Description of Changes
Fixes #3483 and #3487
If the
instance_information.jsonfile does not exists, it starts a local servers and tests against them using Tor Browser.This PR is on top of #3592 PR, so that has to get in first.
Testing
You can run the functional tests like:
Deployment
Any special considerations for deployment? Consider both:
Checklist
If you made changes to the server application code:
make ci-lint) and tests (make -C securedrop test) pass in the development containerIf you made changes to
securedrop-admin:make -C admin test) pass in the admin development containerIf you made changes to the system configuration:
If you made non-trivial code changes:
If you made changes to documentation:
make docs-lint) passed locally