Speed up st2contrib tests #332
Conversation
Before
After
|
|
Oh, and I also disabled CI on PR, since we're already doing it on Push... it's redundant. |
|
Can you direct some of this rage to the main st2 repo as well :) |
|
🔜 |
|
Excellent! I am going to +1 simply looking at the time improvements :) |
|
Nice! |
|
Overall, I like the idea 👍 and we did a similar thing in the past projects I worked on, but it's also worth pointing out that it has limitations. It really only works on simple checks which operate on a single file (e.g. flake8, json lint, yaml lint). I doesn't work on other more complex checks which do cross file stuff or depend on changes from other files (e.g. pylint and tests). In short - file change is just one type of a change and it's a very simplistic one - there are many other types of changes this approach doesn't detect and where we still want to run the tests (dependency is updated / changed, imported file has changed, etc.). Another thing which is also important is that if we get this to work reliably, we should only enable "run only on changed files" for branches / PRs. On master branch we still want to run it on all the files, not just changed one (otherwise bad changes might slip through and the process is not directly reproducible). Most importantly - the biggest offender right now should be the step where we install the dependencies. The tests and pylint step are relatively fast, only thing which takes a while is dependency installation. We do cache dependencies, but it will take a while in the following cases:
In any of those scenarios we still want to run the tests - dependency change could cause pylint or tests to fail. In short - there are many edge cases so I'm not sure this will work reliably. In the past we pretty much always ended up reverting similar changes and simply do "run on all" - it requires a lot of code changes, it's not really directly reproducible and as mentioned above, there are many edge cases and only a couple of places where it's worth doing it. |
I believe the "PR" one is actually the PR merge commit - running the checks after branch has been merged into master. If that's the case, we should still run the check (PR / branch by itself can pass just fine, but it fails in merge into master). |
Yup. I messed up there. Reverted. Now enabled for PRs as well. Thanks! 🙇 |
|
I'll dig in next week and see if we can get some of those changes in without affecting the "correctness" and "reproduciability" of the tests. In this case (st2contrib) it might be possible if we treat the whole pack as an atomic unit - I believe you already do that, but I haven't checked yet (if we ignore changes in the st2 repo since pylint also does cross checking and referencing files from st2 repo), but in st2 repo we need to treat whole repo as an atomic unit so sadly not much we can do with the file / directory change. |
|
@Kami good deal! thanks. |
|
|
||
| .PHONY: configs-check | ||
| configs-check: requirements | ||
| @echo | ||
| @echo "==================== configs-check ====================" | ||
| @echo | ||
| . $(VIRTUALENV_DIR)/bin/activate; find ${ROOT_DIR}/packs/* -name "*.json" -print0 | xargs -0 -I FILENAME ./scripts/validate-json-file.sh FILENAME | ||
| . $(VIRTUALENV_DIR)/bin/activate; find ${ROOT_DIR}/packs/* -name "*.yaml" -print0 | xargs -0 -I FILENAME ./scripts/validate-yaml-file.sh FILENAME | ||
| . $(VIRTUALENV_DIR)/bin/activate; for file in "$(CHANGED_YAML)"; do if [ -n "$$file" ]; then ./scripts/validate-yaml-file.sh $$file; fi; done |
There was a problem hiding this comment.
I'm cherry picking and modifying those changes and I just noticed there is a bug here.
Since the whole string is quoted if there are multiple changed files it will actually only iterate once over the whole string (and not over all the words).
…changed on safe checks. In addition to that we only run if changes in branch - on master branch we still run checks on all the files.
|
Superseded by #343. |
…changed on safe checks. In addition to that we only run if changes in branch - on master branch we still run checks on all the files.
Got a little upset today at the long build times, so I 😡 and fixed it.
Below are updates to the
st2contribrepo, changing up the way that files are parsed. This removes the genericfindglob from each of the tests and replaces with only the files that have changed.Might require a bit of fixups, but hopefully you get the gist.