ci: Disable macOS functional tests on forked repos to avoid timeouts

hebasto commented at 5:42 pm on July 11, 2020: member

See: https://github.com/bitcoin-core/gui/issues/5#issuecomment-656819184

Additionally, this PR:

updates macOS image to the recent 10.15.5 version
drops Homebrew caching as the Travis Homebrew addon have been used since #18438

My forked repo build: https://travis-ci.org/github/hebasto/bitcoin/jobs/707200431

DrahtBot added the label Tests on Jul 11, 2020

DrahtBot commented at 7:25 pm on July 11, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#18077 (net: Add NAT-PMP port forwarding support by hebasto)
#15382 (util: add RunCommandParseJSON by Sjors)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

promag commented at 1:06 am on July 12, 2020: member

I think commit “ci: Update macOS image to 10.15.5 on Travis” doesn’t belong here. What is the motivation? ACK remaining commits.

hebasto force-pushed on Jul 12, 2020

hebasto commented at 8:45 am on July 12, 2020: member

@promag

I think commit “ci: Update macOS image to 10.15.5 on Travis” doesn’t belong here. What is the motivation? ACK remaining commits.

The mentioned commit has been dropped.

promag commented at 8:49 am on July 12, 2020: member

ACK 0b9e1e614c3b4fa747d6c1dbeedef71ce834fe94

MarcoFalke renamed this:
~~ci: Disable functional tests on forked repos to avoid timeouts for macOS~~
ci: Disable macOS functional tests on forked repos to avoid timeouts
on Jul 12, 2020

in .travis.yml:51 in cab825ea9c outdated

47   - export CONTINUE=1
48   - if [ $SECONDS -gt 1200 ]; then export CONTINUE=0; fi  # Likely the depends build took very long
49   - if [ $TRAVIS_REPO_SLUG = "bitcoin/bitcoin" ]; then export CONTINUE=1; fi  # continue on repos with extended build time (90 minutes)
50   - if [ $CONTINUE = "1" ]; then set -o errexit; source ./ci/test/06_script_a.sh; else set +o errexit; echo "$CACHE_ERR_MSG"; false; fi
51-  - if [ $SECONDS -gt 2000 ]; then export CONTINUE=0; fi  # Likely the build took very long; The tests take about 1000s, so we should abort if we have less than 50*60-1000=2000s left
52+  - if [ $SECONDS -gt 2000 && $LONG_RUNNING_TEST = "1" ]; then export CONTINUE=0; fi  # Likely the build took very long; The tests take about 1000s, so we should abort if we have less than 50*60-1000=2000s left

MarcoFalke commented at 9:28 am on July 12, 2020:

Should this be an && or || and why? Also, why use the runtime heuristic in combination with the heuristic in line 46. Wouldn’t it be easier to simply export LONG_RUNNING_TEST for the configs that need it? I.e. right in the line where the other env vars are defined:

0      env: >-
1        FILE_ENV="..."
2        LONG_...

hebasto commented at 10:00 am on July 12, 2020:

^ this will not work for forked repos as the LONG_RUNNING_TEST actually should depend on the TRAVIS_REPO_SLUG.

Another suggestion is to export LONG_RUNNING_TEST="true" by default in 00_setup_env.sh and set it in a job setup script 00_setup_env_*.sh:

0if [ "$TRAVIS_REPO_SLUG" != "bitcoin/bitcoin" ]; then
1  export RUN_FUNCTIONAL_TESTS="false"
2  export LONG_RUNNING_TEST="false"
3fi

MarcoFalke commented at 10:48 am on July 12, 2020:

If you don’t set LONG_RUNNING_TEST to anything for the macos config, everything will work fine in all repos, no?

bitcoin/bitcoin will run the functional tests, and forked repos won’t. Independent of LONG_RUNNING_TEST

hebasto commented at 11:11 am on July 12, 2020:

If you don’t set LONG_RUNNING_TEST to anything for the macos config, everything will work fine in all repos, no?

bitcoin/bitcoin will run the functional tests, and forked repos won’t. Independent of LONG_RUNNING_TEST

With cleared (or failed) cache forked repos could experience timeouts (native macos now, but any build could fail with timeout in the future).

The mean of LONG_RUNNING_TEST is:

it is set – do not continue tests if not enough time remains. This should be done to avoid timeouts in forked repos
it is unset – do not take into account the remaining time, just continue. This should be done in cases of manual disabled tests in forked repos.

Actually, the LONG_RUNNING_TEST only affects forked repos due to the following code: https://github.com/bitcoin/bitcoin/blob/4db44acf2d5d3b40943c94b3960f42838255b7ad/.travis.yml#L51-L52

MarcoFalke commented at 11:42 am on July 12, 2020:

Ok, so this seems to affect more than just macos. Hmm. I am not sure if it is expected to fork the repo, run the tests on a commit and then see a green checkmark when the functional tests haven’t even been run. Then the commit is proposed to bitcoin/bitcoin and the tests fail.

Excluding macOS is fine because I am not aware this ever detected a bug that was not detected by any of the other builds. Though, disabling all tests for forked repos seems a bit too much, no?

If travis can’t give us what we need, we should switch providers instead of disabling tests conditionally.

in .travis.yml:44 in 0b9e1e614c outdated

40@@ -43,11 +41,12 @@ before_script:
41   - for i in {1..4}; do echo "$(sleep 500)" ; done &
42   - set -o errexit; source ./ci/test/05_before_script.sh &> "/dev/null"
43 script:
44+  - if [[ -n "$USE_VALGRIND" || "$RUN_FUNCTIONAL_TESTS" = "true" || "$RUN_FUZZ_TESTS" = "true" ]]; then export LONG_RUNNING_TEST=1; fi

MarcoFalke commented at 9:29 am on July 12, 2020:

are the fuzz tests timing out as well these days?

practicalswift commented at 9:13 pm on July 12, 2020:

I hope not! Do we have any evidence suggesting that? If so I’ll investigate how we could speed things up.

MarcoFalke approved

MarcoFalke commented at 9:29 am on July 12, 2020: member

ACK

MarcoFalke commented at 5:04 am on July 13, 2020: member

withdraw ACK for now

MarcoFalke changes_requested

MarcoFalke commented at 5:04 am on July 13, 2020: member

withdraw ACK for now

hebasto force-pushed on Jul 13, 2020

hebasto commented at 9:22 am on July 13, 2020: member

Updated 0b9e1e614c3b4fa747d6c1dbeedef71ce834fe94 -> 12f642de257a0280e014ad8660f54ee45574a637 (pr19495.02 -> pr19495.03, diff):

timeout changes affect only native macOS build now
added “ci: Fix configure options for macOS builds” commit

–

However, still working on performance improving for the native macOS build on Travis.

in .travis.yml:49 in 12f642de25 outdated

45@@ -48,7 +46,7 @@ script:
46   - if [ $TRAVIS_REPO_SLUG = "bitcoin/bitcoin" ]; then export CONTINUE=1; fi  # continue on repos with extended build time (90 minutes)
47   - if [ $CONTINUE = "1" ]; then set -o errexit; source ./ci/test/06_script_a.sh; else set +o errexit; echo "$CACHE_ERR_MSG"; false; fi
48   - if [ $SECONDS -gt 2000 ]; then export CONTINUE=0; fi  # Likely the build took very long; The tests take about 1000s, so we should abort if we have less than 50*60-1000=2000s left
49-  - if [ $TRAVIS_REPO_SLUG = "bitcoin/bitcoin" ]; then export CONTINUE=1; fi  # continue on repos with extended build time (90 minutes)
50+  - if [ $TRAVIS_REPO_SLUG = "bitcoin/bitcoin" || $CONTINUE_IN_FORKED_REPO = "true" ]; then export CONTINUE=1; fi  # continue on repos with extended build time (90 minutes)

MarcoFalke commented at 9:44 am on July 13, 2020:

0  - if [ $TRAVIS_REPO_SLUG = "bitcoin/bitcoin" ]; then export CONTINUE=1; fi  # continue on repos with extended build time (90 minutes)

Is this needed? With a warm cache, this should already continue, and without a warm cache it might or might not continue.

I think we should either keep track of when to continue manually for all configs or with the time-heuristic for all configs, not a mix of both.

MarcoFalke commented at 9:44 am on July 13, 2020: member

ACK

ci: Disable functional tests on forked repos to avoid timeouts for macOS 2d747428e2

ci: Do not activate Travis ccache caching strategy

It is sufficient to specify CCACHE_DIR to cache.
Also this change fixes ccache on native macOS build.

557d3f1cc0

ci: Drop Homebrew caching while using Homebrew addon on Travis 687939e3d2

ci: Fix configure options for macOS builds 60824b3c3a

hebasto force-pushed on Jul 13, 2020

hebasto commented at 11:45 pm on July 13, 2020: member

Updated 12f642de257a0280e014ad8660f54ee45574a637 -> 60824b3c3a4221a31c3638008b05fecc040c3df2 (pr19495.03 -> pr19495.04):

addressed @MarcoFalke’s comment

I think we should either keep track of when to continue manually for all configs or with the time-heuristic for all configs, not a mix of both.

The time-heuristic is used for all configs. It could customized per build with the EXPECTED_TESTS_DURATION_IN_SECONDS environment variable.

added “ci: Do not activate Travis ccache caching strategy” commit that fixes ccache for native macOS build on Travis

The 300M value of the CCACHE_SIZE variable was chosen after dozens of tests as an optimal one.

MarcoFalke merged this on Jul 14, 2020

MarcoFalke closed this on Jul 14, 2020

hebasto deleted the branch on Jul 14, 2020

sidhujag referenced this in commit 0842b3112b on Jul 14, 2020

DrahtBot locked this on Feb 15, 2022

ci: Disable macOS functional tests on forked repos to avoid timeouts #19495

Conflicts