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

[READY] Removing tox for clarity in CI and tests #42

Merged
merged 4 commits into from
Apr 9, 2021

Conversation

felipejinli
Copy link
Contributor

@felipejinli felipejinli commented Apr 2, 2021

Purpose: This simplifies the setup process by removing tox. NB. Travis already runs tests inside an isolated virtualenv for python #32.
Regarding changes: Documentation builds are run at every push and its output directory has changed from .tox (before) to .docs/_build (after). @asn-d6 Does this cause a conflict (I'm unsure if there's any automated documentation deployment to docs site? Linters are run both for functional and unit tests.

@felipejinli felipejinli marked this pull request as draft April 2, 2021 22:07
@felipejinli felipejinli marked this pull request as ready for review April 2, 2021 22:08
@felipejinli
Copy link
Contributor Author

felipejinli commented Apr 4, 2021

I'm still working on solving an INTERNALERROR when running pytest with coverage reports.
Edit: this has now been fixed and was caused by the mock of datetime.datetime in test_outdated_consensus. Onionbalance never undoes that mock, so datetime.datetime remains a mock object for the rest of the test run. Coverage.py later tries to get the current time to put in its database, and gets a mock object instead of a timestamp. SQLite can't write the mock object to the database.
Fix was to use mock.patch as a context manager that cleans up the mock at the end of the test.

@felipejinli felipejinli changed the title Removing tox for clarity in CI and tests [WIP] Removing tox for clarity in CI and tests Apr 4, 2021
@felipejinli
Copy link
Contributor Author

Pylint is run to only return non-zero exit status if there is an error. However, there are still many Warnings, Convention, and Refactoring messages that should be looked into to maintain high code quality.
@asn-d6 Could you review this pull request?

@felipejinli felipejinli changed the title [WIP] Removing tox for clarity in CI and tests [READY] Removing tox for clarity in CI and tests Apr 5, 2021
@asn-d6
Copy link
Member

asn-d6 commented Apr 5, 2021

Hey @felipejinli ,

thanks a lot for the detailed work here. I'll be reviewing this PR this week. Also, did CI run for your new branch? Can you also link to the CI results of your work?

@felipejinli
Copy link
Contributor Author

@asn-d6 Sounds good. CI build passed (see here)

@asn-d6
Copy link
Member

asn-d6 commented Apr 7, 2021

Hey @felipejinli . Thanks a lot for your work.

I've been trying to run the run-unit-tests.sh script in my Debian box and it says that it can't find pytest even after I did $ sudo pip install -r test-requirements.txt. What am I missing?

Also wrt the pylint stuff you mention in the above comment, what's the difference between flake8 and pylint? I've been using flake8 (as you saw) and I manually ignored a bunch of checks because i felt they were too aggressive.

@felipejinli
Copy link
Contributor Author

felipejinli commented Apr 7, 2021

@asn-d6 That's very strange. Have you checked on your python's site-packages directory to see if pytest is actually installed? Might be helpful if you share the log.
Re pylint and flake8, they're both static code analysers. However, there's sometimes things caught by one but not the other. So, there's no harm in running both of them in our code pipeline.

@asn-d6
Copy link
Member

asn-d6 commented Apr 7, 2021

@asn-d6 That's very strange. Have you checked on your python's site-packages directory to see if pytest is actually installed? Might be helpful if you share the log.
Re pylint and flake8, they're both static code analysers. However, there's sometimes things caught by one but not the other. So, there's no harm in running both of them in our code pipeline.

Hmmm, I did pip install -U pytest and I got it. Sorry about the noise here.

Now, I'm getting this:

$ sh test/scripts/run-functional-tests.sh 
test/scripts/run-functional-tests.sh: 4: Syntax error: "(" unexpected

@felipejinli
Copy link
Contributor Author

Sorry, I'm yet to push the new changes. In the old tox setup, functional tests were not run in Travis CI. I'll push a new commit to fix this soon.

@felipejinli
Copy link
Contributor Author

felipejinli commented Apr 7, 2021

@asn-d6 Could you check the following CI job? Functional tests for both v2 and v3 aren't being run because of the chutney installation process, which doesn't seem to terminate.

@asn-d6
Copy link
Member

asn-d6 commented Apr 7, 2021

Hmmm, I think the v2 functional tests have been broken for a while (because of some chutney changes). Let's just disable them for now and I will remove them for good in a few months when v2 gets deprecated. Sorry for not mentioning that and putting you to needless work.

@felipejinli
Copy link
Contributor Author

@asn-d6 That's sensible. The last Travis build passed (link)

@asn-d6 asn-d6 merged commit aeec1e9 into torproject:master Apr 9, 2021
@asn-d6
Copy link
Member

asn-d6 commented Apr 9, 2021

Merged! Thanks a lot @felipejinli !

I also opened a ticket about reviving the Chutney tests #46

@asn-d6
Copy link
Member

asn-d6 commented Apr 9, 2021

BTW @felipejinli , I tried to revive the v2 functional tests that did not depend on Chutney but now Travis is failing because it can't find onionbalance-config. These work locally in my machine by using pytest but they don't seem to work on travis: https://travis-ci.org/github/asn-d6/onionbalance/jobs/766450099

Any idea what I did wrong?

@felipejinli
Copy link
Contributor Author

@asn-d6 Sorry for the late reply. That's strange, I can't seem to find the cause. But I'll look into it again tomorrow!

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

Successfully merging this pull request may close these issues.

2 participants