-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
MNT CI run all examples if some code is changed #12880
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
Conversation
I don't think it's worthwhile. We'll rely on the tests to detect bugs. |
And as you can see, Circle CI is extremely time-consuming these days. |
Well, it's 7 minutes here (+3min to upload artifacts), unless I'm missing something. Appveyor takes twice as much. I agree that we rely on tests to actually detect issues, but although I value fast CI, I still wonder if just running all of the examples wouldn't be a simpler solution. |
We don't change any code here? Circle CI takes around 40 min in master (and we have 2 jobs). |
It's true that we rely on the tests to detect bugs, but we don't have any systematic way of detecting warnings and issues in the examples to let the authors of PRs fix the examples as well. To me if a PR causes an error in an example, that's an issue needing a fix in that same PR. If you have a better solution to achieve that, I'm happy to hear it. P.S: #12797 (comment) |
Aww, right. I agree 40 min is a lot for each commit in a PR. The summary of options was in #12797 (comment) Honestly I have not seen to many cases where examples would break on master and not in PR, and that would usually mean that the test coverage is not very good. |
Also wouldn't that mean that the PR breaks backward compatibility? I.e. code that worked before now produces an error. This should be very exceptional. |
There was a significant effort to reduce warnings (and fail on warnings) in tests this summer, but if I remember correctly we didn't quite get to fixing it in examples. If I look at the changes in #12654
Overall 112 changed lines needed to fix warnings in examples every 6 month or so doesn't sound too bad. |
Ok. I'm [kinda] convinced. |
Just for the record, this was also discussed in issue #10943. |
The take from the discussion in #12797 is that we'd like to try and run all examples (almost) always.
We also know that running all examples is unnecessary if the PR doesn't touch any code (e.g. documentation or CI related PRs). Therefore this PR does a complete doc build only if there's some change in
.py
,.pyx
, or.pxd
files.An alternative is to remove the part in
get_build_type
which deals with changed files completely, and always return BUILD. But that would have some unnecessary overhead in build time.