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

Use PyTest #746

Merged
merged 2 commits into from
Oct 19, 2018
Merged

Use PyTest #746

merged 2 commits into from
Oct 19, 2018

Conversation

bonjourmauko
Copy link
Member

Technical changes

  • Use pytest to run tests.
  • Declare nose a dependency so dependee libraries can move away from nose.
  • nose is in maintenance mode
  • nose2 was a spin-off of unittest2

Note: openfisca-run-test still depends on nose.

@bonjourmauko bonjourmauko added the kind:refactor Refactoring and code cleanup label Oct 19, 2018
@bonjourmauko bonjourmauko changed the title Use PyLint Use PyTest Oct 19, 2018
@bonjourmauko bonjourmauko requested a review from a team October 19, 2018 16:40
@bonjourmauko bonjourmauko merged commit 73716a7 into master Oct 19, 2018
@bonjourmauko bonjourmauko deleted the use-pytest branch October 19, 2018 17:29
### 24.6.3 [#746](https://github.com/openfisca/openfisca-core/pull/746)

- Use `pytest` to run tests, as `nose` and `nose2` are not in active development anymore
- Declare `nose` as a dependency so dependee libraries like `openfisca-france`can use `openfisca-run-test` without having to add `nose` as a dependency
Copy link
Member

Choose a reason for hiding this comment

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

👍

@@ -21,8 +24,8 @@ format-style:
@# `make` needs `$$` to output `$`. Ref: http://stackoverflow.com/questions/2382764.
autopep8 `git ls-files | grep "\.py$$"`

test: clean check-syntax-errors check-style
nosetests
test: clean check-syntax-errors format-style check-style
Copy link
Member

Choose a reason for hiding this comment

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

⚠️ No, by adding format-style here, we are actually not flaking the code anymore in the CI! ⚠️

Now tests pass with bad style because autopep8 fixes the style before flake8, but the style fix are not commited!

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for catching this!

A slightly longer comment in the main discussion.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops ! Thanks for pointing that out @fpagnoux

@fpagnoux
Copy link
Member

@maukoquiroga @Morendil part of the changes are problematic as they de facto remove code flaking for the CI.

@Morendil
Copy link
Contributor

Hi @fpagnoux and thanks! I'd like to make a comment here at the "learning review" level, as this is not the first time something goes to merge that we have reason to regret afterwards.

Repeated incidents are a good reason to change the conversation from "shit sometimes happens" to "what do we need to change". As you already noted, one item up for discussion is "don't rush to merge", and it's a reasonable position. At least one common characteristic in the two incidents was a short amount of time between PR created and PR merged.

There's a weaker signal that I don't want to get overlooked, which is a tendency to "over-improve", that is, both PRs went beyond the strictly necessary to achieve the stated objective. An alternative approach would have been to split the work beetween one PR to add PyTest, and strictly that, and a second one refactoring the Makefile. I'm all too aware that one factor in that kind of decision is the transaction costs - each PR requires waiting for CI, getting a review, occasionally being stuck for a while on "changes requested", updating Changelog, rebasing, etc. - all of which involve wait times of various lengths.

The larger objective I'm working on at the moment (streamlining contribution, on France in particular) requires a sharper than usual awareness of these various costs from friction, and I make a conscious effort to compensate in the other direction - by approving, fixing what I can, rebasing, updating Changelogs, working at odd hours. In both incidents this (specifically over-eager approve) was an aggravating factor.

@bonjourmauko
Copy link
Member Author

Thanks for your vigilance @fpagnoux

Thanks four your tremendous work @Morendil

Error was carried from openfisca/openfisca-france#1178, my fault.

Repeated incidents are a good reason to change the conversation from "shit sometimes happens" to "what do we need to change". As you already noted, one item up for discussion is "don't rush to merge", and it's a reasonable position. At least one common characteristic in the two incidents was a short amount of time between PR created and PR merged.

👍

There's a weaker signal that I don't want to get overlooked, which is a tendency to "over-improve", that is, both PRs went beyond the strictly necessary to achieve the stated objective. An alternative approach would have been to split the work beetween one PR to add PyTest, and strictly that, and a second one refactoring the Makefile. I'm all too aware that one factor in that kind of decision is the transaction costs - each PR requires waiting for CI, getting a review, occasionally being stuck for a while on "changes requested", updating Changelog, rebasing, etc. - all of which involve wait times of various lengths.

To be completely honest I was lazy and copy pasted the same changes over four pull requests, in an attempt to normalise (style, etc.). Error carried from the first one, that's not yet approved.

The larger objective I'm working on at the moment (streamlining contribution, on France in particular) requires a sharper than usual awareness of these various costs from friction, and I make a conscious effort to compensate in the other direction - by approving, fixing what I can, rebasing, updating Changelogs, working at odd hours. In both incidents this (specifically over-eager approve) was an aggravating factor.

Reducing rotten backlog is awesome. I think this is a good opportunity to assess the maturity we need to give for contributors to help improve others' contributions.

But this is all learning, and we have IMHO a happy problem 😃 .

That being said, I'll clean my dog shit now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:refactor Refactoring and code cleanup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants