Skip to content
This repository has been archived by the owner on Aug 19, 2023. It is now read-only.

Simplify tests: use stdlib, adjust to organization conventions #4

Merged
merged 5 commits into from
Dec 21, 2018

Conversation

diego-plan9
Copy link
Member

Summary

Revise the tests so they only use stdlib features, not requiring additional dependencies in order to be executed. Tweak folders and add docstrings and linting adjusting to the rest of the organization conventions. Update the tox configuration accordingly, using unittest directly for running tests, along with pycodestyle and pylint for linting, in the same spirit as terra, aer and the rest of the organization repositories.

Revise the tests so they only use stdlib features, not requiring
additional dependencies in order to be executed.
Tweak folders and add docstrings and linting adjusting to the rest of
the organization conventions.
Update the tox configuration, using unittest directly for running tests,
along with pycodestyle and pylint for linting.
@mtreinish
Copy link
Member

I'm fine with using stdlib unittest for the tests here we have now, being 2 very quick tests just to sanity check. I only used stestr here to be consistent with what we were doing in terra. If we don't care about that, then we can wait until parallelizing the tests actually makes a difference.

As for pylint I really feel that pylint is far too pedantic and actually impedes productivity instead of improving it by enforcing consistent style (besides not working on py37 properly which is a personal pain). This patch alone is a good example of this things like the docstring rules and other changes made so that pylint will pass don't actually add anything to the code or make it more readable. While the only code in the metapackage is tests I don't think we should force this just because other repos are using it. Especially if we're not going to make other parts of validation consistent with other repos. I also have an open issue about this on terra too: Qiskit/qiskit#1179 to discuss moving away from pylint over there too.

@diego-plan9
Copy link
Member Author

As mention in the description, it's about consistency: I can understand your views on pylint, but I really don't think pointing to an open issue (which was actually informally discussed in a meeting and my recollection is that we were leaning towards not moving away from it) and personal preferences justify deviating from the current practices we have in the project.

@mtreinish
Copy link
Member

Except as I said this doesn't actually make things consistent, the test runner used with this patch is now different from terra. We either care about consistency or we don't. In this specific case I don't think it really buys us much tbh. It's also not like we're consistent across all python projects in the qiskit organization repos. For example, aqua doesn't run any style enforcement, pylint or otherwise, in their ci and other repos have differing rule sets. Also we don't pin pylint versions across repos that do use it. Differing versions of pylint behave very differently.

That all being said we're also only talking about test code (and not a lot of it) here which is fundamentally different from everything else you're comparing against. Enforcing things like every module, class, and function having a docstring does not add anything to test code. (Personally I don't think that kind of enforcement adds much to lib code either, but that is a different argument)

The only reason I mentioned the open issue is to point out this not a settled discussion.

@diego-plan9 diego-plan9 force-pushed the feature/simplify-tests branch 5 times, most recently from 2d6b8ee to 51dbcc0 Compare December 20, 2018 17:50
@diego-plan9 diego-plan9 force-pushed the feature/simplify-tests branch from 51dbcc0 to 6be56fb Compare December 20, 2018 18:25
@delapuente
Copy link
Contributor

As @mtreinish says, let's delay the use of stestr for when parallel tests make a difference which reduces the complexity.

On the other hand, the use of pylint is consistent with the qiskit-terra codebase. This look great to me. Merging.

@delapuente delapuente merged commit ddf9d34 into Qiskit:master Dec 21, 2018
@diego-plan9 diego-plan9 deleted the feature/simplify-tests branch December 21, 2018 10:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants