fix(tests): use TEST_FAKE_ACTIVATION_HEIGHTS at runtime and fix tests (#3749)

* fix: check TEST_FAKE_ACTIVATION_HEIGHTS at runtime

* fix(tests): add TEST_FAKE_ACTIVATION_HEIGHTS variable

This variable ensures the test is activated in the `test-fake-activation-heights` step

* fix(docker): do not run specific tests by default in entrypoint.sh

* fix(test): remove extra TEST_FULL_SYNC argument

* imp(timeout): wait for an average build time

Co-authored-by: Gustavo Valverde <gustavo@iterativo.do>

.github/workflows/cd.yml

@@ -24,7 +24,8 @@ env:
 jobs:
   build:
     name: Build images
-    timeout-minutes: 180
+    # TODO: remove timeout until we have an average build time
+    # timeout-minutes: 180
     runs-on: ubuntu-latest
 
     steps:

.github/workflows/test.yml

@@ -146,7 +146,9 @@ jobs:
       - name: Run tests with fake activation heights
         run: |
           docker pull ${{ env.GAR_BASE }}/${{ env.IMAGE_NAME }}:sha-${{ env.GITHUB_SHA_SHORT }}
-          docker run -e ZEBRA_SKIP_IPV6_TESTS --name zebrad-tests -t ${{ env.GAR_BASE }}/${{ env.IMAGE_NAME }}:sha-${{ env.GITHUB_SHA_SHORT }} cargo test --locked --release --package zebra-state --lib -- with_fake_activation_heights
+          docker run -e ZEBRA_SKIP_IPV6_TESTS -e TEST_FAKE_ACTIVATION_HEIGHTS --name zebrad-tests -t ${{ env.GAR_BASE }}/${{ env.IMAGE_NAME }}:sha-${{ env.GITHUB_SHA_SHORT }} cargo test --locked --release --package zebra-state --lib -- with_fake_activation_heights
+        env:
+          TEST_FAKE_ACTIVATION_HEIGHTS: '1'
 
   # Test that Zebra syncs and checkpoints a few thousand blocks from an empty state
   test-empty-sync:

.github/workflows/zcash-params.yml

@@ -24,7 +24,8 @@ env:
 jobs:
   build:
     name: Build images
-    timeout-minutes: 180
+    # TODO: remove timeout until we have an average build time
+    # timeout-minutes: 180
     runs-on: ubuntu-latest
 
     steps:

docker/Dockerfile

@@ -66,9 +66,6 @@ ENV CHECKPOINT_SYNC ${CHECKPOINT_SYNC:-true}
 ARG NETWORK
 ENV NETWORK ${NETWORK:-Mainnet}
 
-ARG TEST_FULL_SYNC
-ENV TEST_FULL_SYNC ${TEST_FULL_SYNC:-1}
-
 COPY . .
 # Build zebra
 RUN cargo build --locked --release --features enable-sentry --bin zebrad
@@ -87,10 +84,10 @@ RUN chmod u+x /entrypoint.sh
 ARG CHECKPOINT_SYNC=true
 ARG NETWORK=Mainnet
 ARG TEST_FULL_SYNC
-ENV TEST_FULL_SYNC ${TEST_FULL_SYNC:-1}
+ENV TEST_FULL_SYNC ${TEST_FULL_SYNC:-0}
 
-ARG RUN_TESTS
-ENV RUN_TESTS ${RUN_TESTS:-1}
+ARG RUN_ALL_TESTS
+ENV RUN_ALL_TESTS ${RUN_ALL_TESTS:-0}
 
 ENTRYPOINT ["/entrypoint.sh"]
 CMD [ "cargo"]

docker/entrypoint.sh

@@ -19,12 +19,12 @@ fi
 
 case "$1" in
     -- | cargo)
-        if [[ "$RUN_TESTS" -eq "1" ]]; then
-            if [[ "$TEST_FULL_SYNC" -eq "1" ]]; then
-                exec cargo "test" "--locked" "--release" "--features" "enable-sentry" "--test" "acceptance" "--" "--nocapture" "--ignored" "full_sync_mainnet"
-            else
-                exec cargo "test" "--locked" "--release" "--features" "enable-sentry" "--workspace" "--" "--include-ignored"
-            fi
+        if [[ "$RUN_ALL_TESTS" -eq "1" ]]; then
+            exec cargo "test" "--locked" "--release" "--features" "enable-sentry" "--workspace" "--" "--include-ignored"
+        elif [[ "$TEST_FULL_SYNC" -eq "1" ]]; then
+            exec cargo "test" "--locked" "--release" "--features" "enable-sentry" "--test" "acceptance" "--" "--nocapture" "--ignored" "full_sync_mainnet"
+        else
+            exec "$@"
         fi
         ;;
     zebrad)

zebra-chain/build.rs

@@ -1,14 +0,0 @@
-//! Build script for zebra-chain.
-//!
-//! Turns the environmental variable `$TEST_FAKE_ACTIVATION_HEIGHTS`
-//! into the Rust configuration `cfg(test_fake_activation_heights)`.
-
-use std::env;
-
-fn main() {
-    let use_fake_heights = env::var_os("TEST_FAKE_ACTIVATION_HEIGHTS").is_some();
-    println!("cargo:rerun-if-env-changed=TEST_FAKE_ACTIVATION_HEIGHTS");
-    if use_fake_heights {
-        println!("cargo:rustc-cfg=test_fake_activation_heights");
-    }
-}

zebra-chain/src/parameters/network_upgrade.rs

@@ -52,8 +52,13 @@ pub enum NetworkUpgrade {
 ///
 /// This is actually a bijective map, but it is const, so we use a vector, and
 /// do the uniqueness check in the unit tests.
-#[cfg(not(test_fake_activation_heights))]
-pub(crate) const MAINNET_ACTIVATION_HEIGHTS: &[(block::Height, NetworkUpgrade)] = &[
+///
+/// # Correctness
+///
+/// Don't use this directly; use NetworkUpgrade::activation_list() so that
+/// we can switch to fake activation heights for some tests.
+#[allow(unused)]
+pub(super) const MAINNET_ACTIVATION_HEIGHTS: &[(block::Height, NetworkUpgrade)] = &[
     (block::Height(0), Genesis),
     (block::Height(1), BeforeOverwinter),
     (block::Height(347_500), Overwinter),
@@ -64,8 +69,9 @@ pub(crate) const MAINNET_ACTIVATION_HEIGHTS: &[(block::Height, NetworkUpgrade)]
     // TODO: Add Nu5 mainnet activation height
 ];
 
-#[cfg(test_fake_activation_heights)]
-pub(crate) const MAINNET_ACTIVATION_HEIGHTS: &[(block::Height, NetworkUpgrade)] = &[
+/// Fake mainnet network upgrade activation heights, used in tests.
+#[allow(unused)]
+const FAKE_MAINNET_ACTIVATION_HEIGHTS: &[(block::Height, NetworkUpgrade)] = &[
     (block::Height(0), Genesis),
     (block::Height(5), BeforeOverwinter),
     (block::Height(10), Overwinter),
@@ -80,8 +86,13 @@ pub(crate) const MAINNET_ACTIVATION_HEIGHTS: &[(block::Height, NetworkUpgrade)]
 ///
 /// This is actually a bijective map, but it is const, so we use a vector, and
 /// do the uniqueness check in the unit tests.
-#[cfg(not(test_fake_activation_heights))]
-pub(crate) const TESTNET_ACTIVATION_HEIGHTS: &[(block::Height, NetworkUpgrade)] = &[
+///
+/// # Correctness
+///
+/// Don't use this directly; use NetworkUpgrade::activation_list() so that
+/// we can switch to fake activation heights for some tests.
+#[allow(unused)]
+pub(super) const TESTNET_ACTIVATION_HEIGHTS: &[(block::Height, NetworkUpgrade)] = &[
     (block::Height(0), Genesis),
     (block::Height(1), BeforeOverwinter),
     (block::Height(207_500), Overwinter),
@@ -92,8 +103,9 @@ pub(crate) const TESTNET_ACTIVATION_HEIGHTS: &[(block::Height, NetworkUpgrade)]
     (block::Height(1_599_200), Nu5),
 ];
 
-#[cfg(test_fake_activation_heights)]
-pub(crate) const TESTNET_ACTIVATION_HEIGHTS: &[(block::Height, NetworkUpgrade)] = &[
+/// Fake testnet network upgrade activation heights, used in tests.
+#[allow(unused)]
+const FAKE_TESTNET_ACTIVATION_HEIGHTS: &[(block::Height, NetworkUpgrade)] = &[
     (block::Height(0), Genesis),
     (block::Height(5), BeforeOverwinter),
     (block::Height(10), Overwinter),
@@ -170,10 +182,38 @@ impl NetworkUpgrade {
     /// network upgrade does not appear in the list.
     ///
     /// This is actually a bijective map.
+    ///
+    /// When the environment variable TEST_FAKE_ACTIVATION_HEIGHTS is set
+    /// and it's a test build, this returns a list of fake activation heights
+    /// used by some tests.
     pub(crate) fn activation_list(network: Network) -> BTreeMap<block::Height, NetworkUpgrade> {
+        println!(
+            "activation_list called {:?}",
+            std::env::var_os("TEST_FAKE_ACTIVATION_HEIGHTS")
+        );
+        let (mainnet_heights, testnet_heights) = {
+            #[cfg(not(feature = "zebra-test"))]
+            {
+                (MAINNET_ACTIVATION_HEIGHTS, TESTNET_ACTIVATION_HEIGHTS)
+            }
+            // To prevent accidentally setting this somehow, only check the env var
+            // when being compiled for tests. We can't use cfg(test) since the
+            // test that uses this is in zebra-state, and cfg(test) is not
+            // set for dependencies. However, zebra-state does set the
+            // zebra-test feature of zebra-chain if it's a dev dependency.
+            #[cfg(feature = "zebra-test")]
+            if std::env::var_os("TEST_FAKE_ACTIVATION_HEIGHTS").is_some() {
+                (
+                    FAKE_MAINNET_ACTIVATION_HEIGHTS,
+                    FAKE_TESTNET_ACTIVATION_HEIGHTS,
+                )
+            } else {
+                (MAINNET_ACTIVATION_HEIGHTS, TESTNET_ACTIVATION_HEIGHTS)
+            }
+        };
         match network {
-            Mainnet => MAINNET_ACTIVATION_HEIGHTS,
-            Testnet => TESTNET_ACTIVATION_HEIGHTS,
+            Mainnet => mainnet_heights,
+            Testnet => testnet_heights,
         }
         .iter()
         .cloned()

zebra-chain/src/parameters/tests.rs

@@ -14,6 +14,11 @@ use NetworkUpgrade::*;
 fn activation_bijective() {
     zebra_test::init();
 
+    if std::env::var_os("TEST_FAKE_ACTIVATION_HEIGHTS").is_some() {
+        eprintln!("Skipping activation_bijective() since $TEST_FAKE_ACTIVATION_HEIGHTS is set");
+        return;
+    }
+
     let mainnet_activations = NetworkUpgrade::activation_list(Mainnet);
     let mainnet_heights: HashSet<&block::Height> = mainnet_activations.keys().collect();
     assert_eq!(MAINNET_ACTIVATION_HEIGHTS.len(), mainnet_heights.len());

zebra-state/build.rs

@@ -1,14 +0,0 @@
-//! Build script for zebra-state.
-//!
-//! Turns the environmental variable `$TEST_FAKE_ACTIVATION_HEIGHTS`
-//! into the Rust configuration `cfg(test_fake_activation_heights)`.
-
-use std::env;
-
-fn main() {
-    let use_fake_heights = env::var_os("TEST_FAKE_ACTIVATION_HEIGHTS").is_some();
-    println!("cargo:rerun-if-env-changed=TEST_FAKE_ACTIVATION_HEIGHTS");
-    if use_fake_heights {
-        println!("cargo:rustc-cfg=test_fake_activation_heights");
-    }
-}

zebra-state/src/service/finalized_state/tests/prop.rs

@@ -45,10 +45,17 @@ fn blocks_with_v5_transactions() -> Result<()> {
 /// Test if committing blocks from all upgrades work correctly, to make
 /// sure the contextual validation done by the finalized state works.
 /// Also test if a block with the wrong commitment is correctly rejected.
-#[allow(dead_code)]
-#[cfg_attr(test_fake_activation_heights, test)]
+///
+/// This test requires setting the TEST_FAKE_ACTIVATION_HEIGHTS.
+#[test]
 fn all_upgrades_and_wrong_commitments_with_fake_activation_heights() -> Result<()> {
     zebra_test::init();
+
+    if std::env::var_os("TEST_FAKE_ACTIVATION_HEIGHTS").is_none() {
+        eprintln!("Skipping all_upgrades_and_wrong_commitments_with_fake_activation_heights() since $TEST_FAKE_ACTIVATION_HEIGHTS is NOT set");
+        return Ok(());
+    }
+
     // Use no_shrink() because we're ignoring _count and there is nothing to actually shrink.
     proptest!(ProptestConfig::with_cases(env::var("PROPTEST_CASES")
         .ok()