Skip to content
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.

Sign up for GitHub

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

Jump to bottom

move pixel-tests to use git-utils.sh, including caching #15995

Closed
wants to merge 4 commits into from
Closed

move pixel-tests to use git-utils.sh, including caching #15995

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7794bd2
tools: move git-utils.sh to pkg/lib
allisonkarlitskaya Jun 25, 2021
4cae92e
git-utils: move cmd_remove() to git-utils
allisonkarlitskaya Jun 25, 2021
2617f41
pixel-tests: use git-utils.sh
allisonkarlitskaya Jun 25, 2021
4ca93d7
testlib: call `pixel-tests pull` unconditionally
allisonkarlitskaya Jun 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Makefile.am
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ EXTRA_DIST = \
package.json \
package-lock.json \
README.md \
pkg/lib/git-utils.sh \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

tools/debian/copyright \
$(NULL)

Expand Down
11 changes: 11 additions & 0 deletions tools/git-utils.sh → pkg/lib/git-utils.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FTR, :+1 for calling this SSH_REMOTE instead of PUSH_REMOTE, as pushing may happen over https as well.


CACHE_DIR="${XDG_CACHE_HOME-${HOME}/.cache}/cockpit-dev/${GITHUB_REPOSITORY}.git"

Expand Down Expand Up @@ -108,3 +109,13 @@ unpack_from_cache() {
message "UNPACK" "${SUBDIR} [ref: $1]"
git_cache archive "$1" "${SUBDIR}" | tar -x --touch "${SUBDIR}"
}

cmd_remove() {
# if we did this for ourselves the rm is enough, but it might be the case
# that someone actually used git-submodule to fetch this, so clean up after
# that as well. NB: deinit nicely recreates the empty directory for us.
message REMOVE "${SUBDIR}"
rm -rf "${SUBDIR}"
git submodule deinit "${SUBDIR}"
rm -rf "$(git rev-parse --absolute-git-dir)/modules/${SUBDIR}"
}
40 changes: 25 additions & 15 deletions test/common/pixel-tests
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,25 +1,35 @@
#!/bin/bash

set -eu

SUBDIR=test/reference
REPO=pixel-test-reference

GITHUB_BASE="${GITHUB_BASE:-cockpit-project/cockpit}"
GITHUB_REPOSITORY="${GITHUB_BASE%/*}/${REPO}"
CLONE_REMOTE="https://github.com/${GITHUB_REPOSITORY}"
PUSH_REMOTE="git@github.com:${GITHUB_REPOSITORY}"
GITHUB_REPO='pixel-test-reference'
SUBDIR='test/reference'

message() {
[ "${V-}" != 0 ] || printf " %-8s %s\n" "$1" "$2"
}
set -eu
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tiny style nitpick: This ought to stay the first line in this (and every) script. Presumably it just got moved by accident?

cd "$(realpath -m "$0"/../../..)"
. pkg/lib/git-utils.sh

cmd_init() {
git submodule add -b empty "$CLONE_REMOTE" "$SUBDIR"
}

cmd_checkout() {
local sha="${1-$(get_index_gitlink node_modules)}"

fetch_sha_to_cache "${sha}"
cmd_remove
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment above. I'd like your input to decide what to do here :)

clone_from_cache "${sha}"
}

cmd_pull() {
git submodule update --init -- "$SUBDIR"
# Our main goal is to ensure that the test/reference from
# the index is the one that we actually have.
local sha="$(get_index_gitlink test/reference)"
if [ ! -e test/reference/.git ]; then
# nothing there yet...
cmd_checkout "${sha}"
elif [ "$(git -C test/reference rev-parse HEAD)" != "${sha}" ]; then
# wrong thing there...
cmd_checkout "${sha}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks a bit odd to have two branches with exactly the same code. This could just || the condition.

fi
}

cmd_status() {
Expand All @@ -45,7 +55,7 @@ cmd_push() {
fi
tag="sha-$(git rev-parse HEAD)"
[ $(git tag -l "$tag") ] || git tag "$tag" HEAD
git push "$PUSH_REMOTE" "$tag"
git push "$SSH_REMOTE" "$tag"
)
git add "$SUBDIR"
}
Expand All @@ -62,7 +72,7 @@ main() {
fi

shift
[ "${V-0}" = 0 ] || set -x
[ -n "${quiet}" ] || set -x
"cmd_$cmd" "$@"
}

Expand Down
9 changes: 4 additions & 5 deletions test/common/testlib.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll implement this. Thanks.


self.call_js_func('ph_scrollIntoViewIfNeeded', selector)

rect = self.call_js_func('ph_element_clip', selector)
Expand All @@ -673,14 +676,10 @@ def relative_clip(sel):
r['x'] - rect['x'] + r['width'],
r['y'] - rect['y'] + r['height'])

reference_dir = os.path.join(TEST_DIR, 'reference')
if not os.path.exists(os.path.join(reference_dir, '.git')):
subprocess.check_call([f'{TEST_DIR}/common/pixel-tests', 'pull'])

ignore_rects = list(map(relative_clip, map(lambda item: selector + " " + item, ignore)))
base = self.pixels_label + "-" + key
filename = base + "-pixels.png"
ref_filename = os.path.join(reference_dir, filename)
ref_filename = os.path.join(TEST_DIR, 'reference', filename)
ret = self.cdp.invoke("Page.captureScreenshot", clip=rect, no_trace=True)
png_now = base64.standard_b64decode(ret["data"])
png_ref = os.path.exists(ref_filename) and open(ref_filename, "rb").read()
Expand Down
2 changes: 1 addition & 1 deletion tools/make-bots
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ V="${V-0}" # default to friendly messages

set -eu
cd "$(realpath -m "$0"/../..)"
. tools/git-utils.sh
. pkg/lib/git-utils.sh

if [ ! -d bots ]; then
[ -n "${quiet}" ] || set -x
Expand Down
12 changes: 1 addition & 11 deletions tools/node-modules
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,7 @@ SUBDIR='node_modules'

set -eu
cd "$(realpath -m "$0"/../..)"
. tools/git-utils.sh

cmd_remove() {
# if we did this for ourselves the rm is enough, but it might be the case
# that someone actually used git-submodule to fetch this, so clean up after
# that as well. NB: deinit nicely recreates the empty directory for us.
message REMOVE node_modules
rm -rf node_modules
git submodule deinit node_modules
rm -rf "$(git rev-parse --absolute-git-dir)/modules/node_modules"
}
. pkg/lib/git-utils.sh

cmd_checkout() {
# we default to check out the node_modules corresponding to the gitlink in the index
Expand Down
2 changes: 1 addition & 1 deletion tools/webpack-jumpstart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ export V="${V-0}"

set -eu
cd "$(realpath -m "$0"/../..)"
. tools/git-utils.sh
. pkg/lib/git-utils.sh

[ -n "${quiet}" ] || set -x

Expand Down