move pixel-tests to use git-utils.sh, including caching #15995
Conversation
allisonkarlitskaya
force-pushed
the
pixel-tests-git-utils
branch
from
49ea37d
to
4ca93d7
Compare
This allows the utility functions to be used from other projects, which tend to check out pkg/lib into their own tree. That will enable us to use the caching mechanics for the pixel test reference images. This isn't the perfect place for this file, but neither was tools/. This is definitely a better place than test/common/. In general, this seems to be the "least bad" location.
We're about to start using this also from pixel-tests.
Reimplement cmd_pull() based on the caching code from git-utils.sh.
This should be very fast now, in the "already checked out" case, particularly compared with the sort of operations that are normally taking place in our integration testing.
There was a problem hiding this comment.
Thanks! A few tiny style nitpicks. As this adds a whole lot of new code, the commit message could do with some rationale. Supposedly this improves/introdudes the local caching, whereas the original script always pulled it fresh from the internet?
Let's give @mvollmer a chance to review this. If he doesn't get to it before PTO, I think I feel reasonably qualified to ack/land this 😁
| @@ -28,6 +28,7 @@ EXTRA_DIST = \ | |||
| package.json \ | |||
| package-lock.json \ | |||
| README.md \ | |||
| pkg/lib/git-utils.sh \ | |||
There was a problem hiding this comment.
Is it actually important to ship this now, when we haven't so far? (FTR, I'm fine with it, just wondering if that is coincidental, or deliberate)
| @@ -10,6 +10,7 @@ | |||
| GITHUB_BASE="${GITHUB_BASE:-cockpit-project/cockpit}" | |||
| GITHUB_REPOSITORY="${GITHUB_BASE%/*}/${GITHUB_REPO}" | |||
| HTTPS_REMOTE="https://github.com/${GITHUB_REPOSITORY}" | |||
| SSH_REMOTE="git@github.com:${GITHUB_REPOSITORY}" | |||
There was a problem hiding this comment.
FTR, :+1 for calling this SSH_REMOTE instead of PUSH_REMOTE, as pushing may happen over https as well.
| message() { | ||
| [ "${V-}" != 0 ] || printf " %-8s %s\n" "$1" "$2" | ||
| } | ||
| set -eu |
There was a problem hiding this comment.
Tiny style nitpick: This ought to stay the first line in this (and every) script. Presumably it just got moved by accident?
| cmd_checkout "${sha}" | ||
| elif [ "$(git -C test/reference rev-parse HEAD)" != "${sha}" ]; then | ||
| # wrong thing there... | ||
| cmd_checkout "${sha}" |
There was a problem hiding this comment.
Looks a bit odd to have two branches with exactly the same code. This could just || the condition.
There was a problem hiding this comment.
Approved, since I mostly care about the status and push sub-commands. They are unique to the pixel-tests workflow, I'd say, and if we make pull or checkout more similar to the rest of our tools, I can't disagree.
I was taking the position that if you know submodules, test/common/pixel-tests should offer no surprises for you. This might no longer be true with this change, but I can't say I have a strong opinion. We should update the blog post if necessary.
| @@ -662,6 +662,9 @@ def assert_pixels(self, selector, key, ignore=[]): | |||
| if not (Image and self.pixels_label and self.cdp and self.cdp.valid): | |||
| return | |||
|
|
|||
| # Make sure everything is up to date. This is normally very fast. | |||
| subprocess.check_call([f'{TEST_DIR}/common/pixel-tests', 'pull']) | |||
There was a problem hiding this comment.
This runs the risk of removing anything that is in test/reference/ and checking it out from scratch, right? That might remove files that the dev has moved into test/reference while working on new pixel tests, no?
There was a problem hiding this comment.
This is one of the reasons I wanted your review here. What do you want to do about this? Refuse to delete the old tree if it's not clean?
There was a problem hiding this comment.
Refuse to delete the old tree if it's not clean?
Yes, that sounds good. Also, have a --force option for pull to delete it anyway.
There was a problem hiding this comment.
I'll implement this. Thanks.
| local sha="${1-$(get_index_gitlink node_modules)}" | ||
|
|
||
| fetch_sha_to_cache "${sha}" | ||
| cmd_remove |
There was a problem hiding this comment.
I am not sure I like how pixel-tests pull might now delete test/reference completely. This will remove untracked files, while git checkout and git submodule update leave those alone.
On the one hand, removing untracked files will make it impossible for the pull to fail because of conflicts, on the other hand, it might throw files away that the developer cares about.
There was a problem hiding this comment.
See comment above. I'd like your input to decide what to do here :)
martinpitt
dismissed
mvollmer’s stale review
outdated, challenged, needs rebasing now
|
We can bring this back later at some point, if we want. |
No description provided.