Updates README with better instructions for test data #129
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The updates are a big improvement! If I follow these instructions for re-generating all the proxy test data at the same time, steps 1-2 work, but step 3 (uncommenting the dastollervey decorators and rerunning make test TESTS=tests/test_apiproxy.py
) always has 6 failing tests for me, each with a 403 error. I believe this is because these tests are trying to use an old TOTP code. It looks like to get around this, we'll need to add a step 4: restart the server, step 5: rerun the tests until each failing test eventually passes, and if you have to just rerun a failing test individually by itself. We'll also need to add a section for TestAPI
where you run the server local.
I'll make the changes I mentioned above and approve.
README.md
Outdated
``` | ||
**Note:** If you get a 403 error it's becuase the test is trying to reuse an old TOTP code, so wait for 60 seconds and try again. Some tests alter source and conversation data on the server so you should restart the server in between test runs. | ||
|
||
**Note:** Remember that file download checks don't read actual file path in the `APIProxy` tests as it requires QubesOS setup. You can manually uncomment those lines to execute them on QubesOS setup. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kushaldas: I'm not sure what you're saying here. Are you saying that the tests that check whether or not a file downloads successfully from the server will fail when regenerating a cassette because it doesn't check the actual file path? I think I hit this but restarting the server and rerunning the tests seemed to make it work. But I don't understand. Could you clarify or perhaps we could modify the file-download tests to pass the first time we generate new cassettes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will have to add the steps to modify /etc/qubes-rpc/qubes.Filecopy
file to add the following so that the proxy can send in the files properly to the sdk.
dev-server dev-client allow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i see now what you're saying. good catch!
@kushal your instructions are great, so I just did a bunch of reorganizing and some rewriting as we discussed. Was hoping you could review for correctness, typos, organization, etc. I think it's close to ready to merge. |
07adc08
to
30a2846
Compare
``` | ||
|
||
5. Uncomment the `@dastollervey_datasaver` decorator wherever you commented it out. | ||
6. Record new cassettes from the response data collected in step 4: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have to restart the server before step 6 iirc.
README.md
Outdated
```bash | ||
make test TESTS=tests/test_apiproxy.py | ||
``` | ||
**Note:** If you get a 403 error it's becuase the test is trying to reuse an old TOTP code, so wait for 60 seconds and try again. Some tests alter source and conversation data on the server so you should restart the server in between test runs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good.
README.md
Outdated
``` | ||
**Note:** If you get a 403 error it's becuase the test is trying to reuse an old TOTP code, so wait for 60 seconds and try again. Some tests alter source and conversation data on the server so you should restart the server in between test runs. | ||
|
||
**Note:** Remember that file download checks don't read actual file path in the `APIProxy` tests as it requires QubesOS setup. You can manually uncomment those lines to execute them on QubesOS setup. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will have to add the steps to modify /etc/qubes-rpc/qubes.Filecopy
file to add the following so that the proxy can send in the files properly to the sdk.
dev-server dev-client allow
2. Install the lastest proxy package: | ||
|
||
```bash | ||
wget https://apt.freedom.press/pool/main/s/securedrop-proxy/<latest-package>.deb |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wget
will fail in the template vm, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i updated this so that we don't recommend maintaining your own template (even though this is what i do). so now we say to run wget to get the latest proxy package in the appvm directly.
30a2846
to
eba769b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was a joint effort between me and kushal. we agreed to go ahead and merge after i addressed his changes.
Closes #128
Contains new instructions for
how to generate test data in Qubes
. I am wondering if I should modify the name of the vms too in this PR.How to test?
Go through the steps and see if you can generate new test data.