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

Tox #67

Merged
merged 8 commits into from
Dec 1, 2020
Merged

Tox #67

merged 8 commits into from
Dec 1, 2020

Conversation

BorjaEst
Copy link
Collaborator

@BorjaEst BorjaEst commented Nov 24, 2020

Create tox configuration with at least the following:

  • Common testenv
  • Cover testenv
  • Pep8 testenv

@BorjaEst
Copy link
Collaborator Author

I cherry picked the basic setup.cfg from #59, but we can merge the final version once it is finished.
For the moment this is only a start with a basic tox configuration (or the idea).

@BorjaEst BorjaEst linked an issue Nov 24, 2020 that may be closed by this pull request
@BorjaEst
Copy link
Collaborator Author

In my computer (using conda) tox is having some troubles to install uWSGI.

    lto1: fatal error: bytecode stream in file ‘/home/borja/miniconda3/envs/flask/lib/python3.8/config-3.8-x86_64-linux-gnu/libpython3.8.a’ generated with LTO version 6.0 instead of the expected 8.1
    compilation terminated.
    lto-wrapper: fatal error: gcc returned 1 exit status
    compilation terminated.
    /home/borja/miniconda3/envs/flask/compiler_compat/ld: error: lto-wrapper failed
    collect2: error: ld returned 1 exit status
    *** error linking uWSGI ***
    ----------------------------------------

Probably, uWSGI is not needed to run nor to unit test, so, it might be possible to drag it out of the minimal installation and put them into a "requirements-prod.txt" or similar.

@BorjaEst
Copy link
Collaborator Author

At this point it seems some tests are failing:

$ tox
...
eosc_perf/tests/facade_test.py ....................................                                  [ 35%]
eosc_perf/tests/io_controller_test.py ......F...F..........F....F...............................     [ 93%]
eosc_perf/tests/json_result_validator_test.py .......                                                [100%]
...

For the moment we can leave them (investigate later).
Now I would suggest to continue on the development of tox.ini to add coverage and other features.

@BorjaEst
Copy link
Collaborator Author

Tests for pep8 added. An output file is produced "flake8.log" after running $ tox. There are some recomendations, but can be fixed in long term.

@BorjaEst BorjaEst requested a review from vykozlov November 24, 2020 08:51
@BorjaEst BorjaEst added the code improvement something works but should be reworked in some way label Nov 24, 2020
@BorjaEst BorjaEst mentioned this pull request Nov 24, 2020
2 tasks
@TheChristophe
Copy link
Collaborator

TheChristophe commented Nov 24, 2020

Probably, uWSGI is not needed to run nor to unit test, so, it might be possible to drag it out of the minimal installation and put them into a "requirements-prod.txt" or similar.

But uwsgi is needed by the application to run in the docker container, which does use setup.cfg, so it should stay in there I think.

To address your specific issue, I don't know much about conda, but it seems it's using your system's compiler, which is too old. Have you tried it with installing gcc through conda, like here? ContinuumIO/anaconda-issues#6619 (comment)

@BorjaEst BorjaEst marked this pull request as ready for review November 26, 2020 10:02
@BorjaEst BorjaEst marked this pull request as draft November 26, 2020 10:04
@BorjaEst
Copy link
Collaborator Author

Still missing the coverage, but for the rest is working

@TheChristophe
Copy link
Collaborator

I have removed the PR from the project board; we have the issue on the project board and that links to the PR, and if we merge the PR the issue gets closed too so it's a little redundant to have both.

tox.ini Outdated
# use en_US.UTF-8 as C.UTF-8 doesn't exist in RHEL7
setenv =
VIRTUAL_ENV={envdir}
LC_ALL=en_US.UTF-88
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

en_US.UTF-8 typo?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ooops, yes

@TheChristophe
Copy link
Collaborator

Do you intend to implement the cover part or should I have a look at it (if you tell me what you mean by it)?

@BorjaEst
Copy link
Collaborator Author

I meant cover for test coverage.
Currently I am on it, trying to call coverage from tox.

Valentin know better tox, but I am trying to undestand how it exaclty works with coverage.

@BorjaEst
Copy link
Collaborator Author

To run coverage: $ tox -e cover
From myside is ready to merge.

More tox env can be added later.

As main inconvinience, I could not run test in parallel without renaming the tests.... If it is a problem we can roll back I keep searching how to use the regex option on stestr, however, I think this rename will save issues on the future (as it took me 4h to find out where the issue was).

@BorjaEst BorjaEst marked this pull request as ready for review November 30, 2020 14:12
@TheChristophe
Copy link
Collaborator

TheChristophe commented Dec 1, 2020

I've tested it and it seems to work. It seems to be missing tox in any of the requirements files however, that may be worth adding.
I'll (re-?)rebase this onto master branch because the files changed and commit history seems a bit confused and I'm not a fan of unclear git spaghetti.

@TheChristophe
Copy link
Collaborator

If I did this correctly, this should be all the current commits. I've also noticed cover is not in the [tox]envlist, so it's not run by default when running tox; is that intended?

@BorjaEst
Copy link
Collaborator Author

BorjaEst commented Dec 1, 2020

Yes, this is in order to avoid running tests twice.
To run only coverage: $ tox -e cover

@BorjaEst BorjaEst merged commit 2e7ffe5 into master Dec 1, 2020
@TheChristophe
Copy link
Collaborator

We may still want to add tox to requirements-test.txt as it's currently not present in any of the requirements I think

@TheChristophe TheChristophe deleted the tox branch December 9, 2020 02:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code improvement something works but should be reworked in some way
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integrate tox framework for unit testing
2 participants