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

tests: increase timeout for unit tests on focal #14876

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

tests: increase timeout for unit tests on focal #14876

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ad9651f
tests: increase timeout for unit tests on focal
maykathm Dec 18, 2024
d476835
tests: scale back timeout for settle
maykathm Dec 19, 2024
37626be
tests: changed go unit test timeout to 15m for all systems
maykathm Dec 19, 2024
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
2 changes: 1 addition & 1 deletion overlord/snapstate/snapstate_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ func (s *snapmgrBaseTest) settle(c *C) {
s.state.Unlock()
defer s.state.Lock()

err := s.o.Settle(testutil.HostScaledTimeout(5 * time.Second))
err := s.o.Settle(testutil.HostScaledTimeout(15 * time.Second))
Copy link
Member

Choose a reason for hiding this comment

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

This change seems unrelated to the TIMEOUT changes below, as this is 5->15 seconds for settle to converge, while below is 5->15 minutes for overall test run. Does the overall test runtime scale with the settle runtime? Or does HostScaledTimeout scale with the TIMEOUT environment variable?

I think this is what's confusing me: if total test runtime scales with settle timeout, and settle timeout is being increased universally, shouldn't total test runtime also be increased universally? And if not, why increase the total test runtime for just focal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Excellent questions. Their identical numbers were actually a coincidence. I was experimenting with how much time focal (because this is only failing on the google focal machine) required to pass its tests. When I increased the settle time, rather than die due to not converging, the tests would reach the overall unit test timeout. So I keep increasing each of those two values by five until the tests were able to pass. It looks like I overshot the settle though. I just dialed that back by 5 seconds and focal is still happy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure if I should increase the overall time for everybody since the failure only occurs on focal. I am generally very much so in favor of removing ifs. So if you think it would be better, I am happy to remove that condition.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm I'm torn about increasing the unit test timeout universally. On the one hand, unit tests ought to take the same amount of time on all systems (else they're not very "unit") (also, why is it taking focal 3x as long??), but on the other hand, if there's a deadlock in a unit test, the runner will sit spinning as long as the timeout lets it, so it could just be burning money. Deadlocks shouldn't be happening though, and local testing before pushing would find them, so I think I'm in favor of universally increasing timeouts, especially since this is for running unit tests inside spread, where runners may be glacial.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the issue has to do with the machine itself (Perhaps network connectivity? Perhaps the CPU is overloaded?). I tried using various focal VMs (both lxd and qemu) and had no issues whatsoever; the unit tests completed in roughly the same amount of time as it takes on my native system.

It would be really nice to not mix the particularities of our CI environment with the codified correct behavior of snapd. That does muddy the water and makes it hard to understand why tests do what they do. It would be awesome to extract these kind of things into configuration files or something that are specific to the backend.

if err != nil {
s.state.Lock()
defer s.state.Unlock()
Expand Down
5 changes: 3 additions & 2 deletions run-checks
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ COVERMODE=${COVERMODE:-atomic}
COVERAGE_SUFFIX=${GO_BUILD_TAGS:-notags}
COVERAGE_OUT=${COVERAGE_OUT:-.coverage/coverage-$COVERAGE_SUFFIX.cov}
CHANGED_FILES=${CHANGED_FILES:-""}
TIMEOUT=${TIMEOUT:-5}

if [ -z "${GITHUB_WORKFLOW:-}" ]; then
# when *not* running inside github, ensure we use go-1.18 by default
Expand Down Expand Up @@ -434,15 +435,15 @@ if [ "$UNIT" = 1 ]; then

tags=
race=
timeout="5m"
timeout="${TIMEOUT}m"
if [ -n "${GO_BUILD_TAGS:-}" ]; then
echo "Using build tags: $GO_BUILD_TAGS"
tags="-tags $GO_BUILD_TAGS"
fi
if [ -n "${GO_TEST_RACE:-}" ]; then
echo "Using go test -race"
race="-race"
timeout="10m"
timeout="$((TIMEOUT*2))m"
fi

echo Building
Expand Down
7 changes: 7 additions & 0 deletions tests/unit/go/task.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,11 @@ execute: |
PROXY_PARAM="$PROXY_PARAM HTTPS_PROXY=$https_proxy"
fi

TIMEOUT=""
if os.query is-focal; then
TIMEOUT=15
fi

if not os.query is-trusty; then
if [ "$VARIANT" = "static" ] ; then
tests.session -u test exec sh -c "cd /tmp/static-unit-tests/src/github.com/snapcore/snapd && \
Expand All @@ -84,6 +89,7 @@ execute: |
$PROXY_PARAM \
SKIP_COVERAGE=1 \
CC=$VARIANT \
TIMEOUT=$TIMEOUT \
./run-checks --unit"
fi
else
Expand Down Expand Up @@ -119,6 +125,7 @@ execute: |
$PROXY_PARAM \
SKIP_COVERAGE=1 \
CC=$VARIANT \
TIMEOUT=$TIMEOUT \
./run-checks --unit" test
fi
fi
Loading