-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Only run tests for modules affected by a PR #2277
Comments
Seems like overkill: all 2160 unit tests run in under 4 seconds on my machine. Now, if we could prune the system tests, that would be a win (but only after merge, of course). |
Yes "Only run tests" was meant to include all tests (maybe unit tests not relevant as you point out) |
@dhermes in python-docs-samples we make nox gather a list of changed files and then filter the list of samples to test based on that. Maybe some of that code could be useful to you? |
Ha, some bubble gum there for sure. When I tried to use the |
Yep, that's why we have a fallback that uses HEAD. On Thu, Sep 8, 2016, 3:38 PM Danny Hermes notifications@github.com wrote:
|
This is sniffing around towards googleapis#2277, to see how much info we can use to restrict the tests we run.
This is sniffing around towards googleapis#2277, to see how much info we can use to restrict the tests we run.
I wanted to summarize how
|
This is sniffing around towards googleapis#2277, to see how much info we can use to restrict the tests we run.
All system tests should be run before a release is tagged. I think the danger of some change affecting another module in some way would outweigh the speed savings. |
That's why I suggested building the dependency graph. |
I'm skeptical that a dependency graph would catch everything all the time. Just seems like adding risk? I may just be paranoid. |
Yes you are definitely paranoid. If code A doesn't depend on code B (whether directly, or indirectly, as traced along the dependency graph) then code B can be completely changed without ever making an impact on the performance of code A (with lots of caveats, e.g. code B can't delete the OS). |
Yeah I understand. I withdraw my concerns based on your confidence. Just make sure whatever makes the graph works with namespaced packages 😉! |
Relying on a third-party dependency-graph analysis tool does add risks: if it has bugs, we could end up not testing code which actually is affected by a change. |
Worth noting that |
This is sniffing around towards googleapis#2277, to see how much info we can use to restrict the tests we run.
This is sniffing around towards googleapis#2277, to see how much info we can use to restrict the tests we run.
This is sniffing around towards googleapis#2277, to see how much info we can use to restrict the tests we run.
Did you close this because the potential benefit doesn't seem to be worth the effort, or because this problem was solved through some other PRs? We're dealing with the same question in our project, so I was hoping to borrow ideas =) |
@pkch We have mostly solved it by examining the files in the new commits. I am trying to make this into a generic helper in http://ci-diff-helper.readthedocs.io/en/latest/ if you want to take a look. |
Inspired by googleapis/google-cloud-node#1579
This may require building a directed graph (based on direct imports) and traversing the graph. We could use
networkx
for this, or maybepylint
already has tooling for dependency graphs.The text was updated successfully, but these errors were encountered: