-
Notifications
You must be signed in to change notification settings - Fork 75
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
Run country & extension template tests systematically #1020
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @maukoquiroga! 👏
This works for me with pip install openfisca-core[dev] && make test
, I confirm that all of Core, Country Template and Extension Template tests are run.
This adds a risk: if Country or Extension template tests are broken, Core tests will break as well even though this would not be related to Core changes. I believe it is an acceptable risk considering that these repository have CI and should never fail anyway, against the added value of checking that Core changes don't break the templates.
37b8e25
to
a48bdd0
Compare
58a02bf
to
2c16d99
Compare
2c16d99
to
f1ade79
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for having taken my suggestions into account 🙂
However, the changeset that was originally reviewed has considerably increased, and this PR seems to have suffered from some feature creep… What was initially a way to “run country & extension template tests systematically” now seems to offer an integral rewrite of the makefile and its logs.
This is exciting, but unfortunately means many more checks have to be done, notably on the cross-platform aspect since some shell-specific syntax is used. It would have to be checked against Windows console, CI, zsh-enabled macOS… Could we please refocus this PR on its original scope? I understand this might be frustrating 😟 but I would really love to see the original proposal land in Core, while minimising risks and enabling a proper conversation around build tools evolution 🙂
Ok with the feature creep 😄 , I'll split things up. |
Thanks @maukoquiroga! |
f1ade79
to
3bb1d88
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I'm still a bit appalled at the complexity of the Makefile syntax, I must say, but thank you so much for mastering it and improving it!
84ffd7f
to
d41188a
Compare
Hello @MattiSG @maukoquiroga : this PR stands in the way of some other PRs. |
I think it is safe to merge this one. |
@MattiSG does it LGTM for you ? |
5e2c70d
to
db4ec8c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
9234668
to
6853af9
Compare
6723e3e
to
d01cb51
Compare
78045eb
to
fccd684
Compare
fccd684
to
e1aafc0
Compare
I am surprised to discover that my approving review was dismissed two weeks ago. Review time is precious, and I feel frustrated that I have to review one more time and that this PR was involuntarily blocked for so long. @maukoquiroga according to the GitHub logs, my review was dismissed through a commit of yours. Did you have any specific action towards that goal? If so, could you share what was your intention? 🙂 If not, do you have an idea why your commit dismissed my review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed & approved again 😉
As seen over the phone with @maukoquiroga, the experiment of auto-dismissing stale reviews started after #990 turns out to be too much of a hassle in our process where we almost systematically rebase before merging and where reviews are very asynchronous. |
Relates to #1021 openfisca/openfisca-france#1649
Technical changes
openfisca test
😉