search

okfn-brasil / serenata-toolbox

📦 pip module containing code shared across Serenata de Amor's projects | ** Este repositório não recebe atualizações frequentes **
MIT License
154 stars 69 forks source link

Introduces pytest to run all tests #91

Closed lipemorais closed 7 years ago

lipemorais commented 7 years ago

What is the purpose of this Pull Request?

The purpose of this PR is make tests run with a more robust test runner to address part of #87 issue.

What was done to achieve this purpose?

How to test if it really works?

To test it just have coveralls installed python -m pip install coveralls and run the code below:

coverage run -m pytest tests/unit
coverage run -m pytest tests/journey

Who can help reviewing it? @anaschwendler @cuducos @jtemporal

lipemorais commented 7 years ago

Sure! I just reordered it.

I also added the travis_wait before the command to run the tests. It should fix the CI.

lipemorais commented 7 years ago

Now Travis CI Pipeline is green. :+1:

cuducos commented 7 years ago

I couldn't test it yet but following the last commits I'd add two questions:

lipemorais commented 7 years ago

If we're changing the way we test the package we need to update README.rst

Sure! I updated README showing how to run tests using setup.py

With this new implementation do we have to document different tests commands (eg how to run only unit tests instead f the whole suite)?

Nice! I added how to run unit and journey tests separated on README

lipemorais commented 7 years ago

@cuducos is there any change that have to be done to make this PR mergeable?

cuducos commented 7 years ago

@cuducos is there any change that have to be done to make this PR mergeable?

Gonna make a new code review, starting now. I spotted some possible changes I'd like to test before merging…

lipemorais commented 7 years ago

I added some inline comments — we cannot rely on pytest command when pytest is installed for tests only

We will need to add the instructions to install it with pip, as we do with isort and prospector. Fixed here 37444b900e049e928e3b57d9e6587e6aa715f6cf

Besides that I couldn't figure out why there are so many changes in test files themselves in this PR. Can you clarify (or, eventually, cleanup and leave this for a new PR)?

Some unit tests were not been executed because was not under tests/unit folder, so I moved them to the unit folder.

No need for the ./ here. coverage run setup.py test is enough ; )

Thanks! Fixed in 1ce5a99aa9f42cf957744db75a67a62f98969876.

Have you tried an envvar approach to separate unit test from integration tests? Something like: python setup.py test for unit tests and RUN_INTEGRATION_TESTS=1 python setup.py test for integration tests?

It seams a little confusing because the second command,RUN_INTEGRATION_TESTS=1 python setup.py test will be no able to run just integrations tests, it will run unit + integration. I think that is not simple and explicit. I feel that with an approach like this we will walk back in the timeline when the integration tests where not running on CI.

What do you think @cuducos ?

lipemorais commented 7 years ago

@cuducos @jtemporal @Irio @anaschwendler

Is there any point that should improved to make this PR able to be merged that was not addressed?

cuducos commented 7 years ago

I must say that in general this PR is missing its purpose:

The purpose of this PR is make tests run with a more robust test runner to address part of #87 issue.

IMHO using pytest only as a short way to call tests has nothing to do with #87. The complexity mentioned in that issue is related to architecture, not with test runner. Problematic architecture (package architecture or tests architecture—or even both, which I think is the case) will result in unorganized and complex code whether it is with unittest or pytest.

Surely I mentioned pytest in #87:

I think that this refactor will enhance our code quality and architecture and can pave the way to more overarching changes such as:

  • [ …]
  • changing the tests suite for something more robust such as pytest, or even using tox

So I truly believe changing the test runner/suite should come in a better moment, that is to say: after refactoring the toolbox, not as a way to refactor it.

That said I'll leave it to another code reviewer, but my vote is to close this issue and address marginal pros it has in another, atomic PRs (for example: run integration and unit tests in parallel in CI, remove of duplicate logic, code style and clean up).