From 8ab47f4f3739d142267e10be66f22d481d0c1639 Mon Sep 17 00:00:00 2001 From: Jelte Fennema Date: Fri, 3 Jul 2020 11:34:55 +0200 Subject: [PATCH] Add a CI check to see if all tests are part of a schedule (#3959) I recently forgot to add tests to a schedule in two of my PRs. One of these was caught by review, but the other one was not. This adds a script to causes CI to ensure that each test in the repo is included in at least one schedule. Three tests were found that were currently not part of a schedule. This PR adds those three tests to a schedule as well and it also fixes some small issues with these tests. --- .circleci/config.yml | 6 ++++ ci/README.md | 16 ++++++++++ ci/check_all_ci_scripts_are_run.sh | 29 +++++++++++++++++++ ci/check_all_tests_are_run.sh | 25 ++++++++++++++++ src/test/regress/Makefile | 5 ++++ .../expected/isolation_cancellation.out | 6 ---- .../multi_create_table_new_features.out | 4 +-- src/test/regress/isolation_schedule | 1 + src/test/regress/multi_schedule | 2 +- src/test/regress/mx_minimal_schedule | 9 ++++++ .../sql/multi_create_table_new_features.sql | 2 +- 11 files changed, 95 insertions(+), 10 deletions(-) create mode 100755 ci/check_all_ci_scripts_are_run.sh create mode 100755 ci/check_all_tests_are_run.sh create mode 100644 src/test/regress/mx_minimal_schedule diff --git a/.circleci/config.yml b/.circleci/config.yml index d640cbf91..fc54424a9 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -53,6 +53,12 @@ jobs: - run: name: 'Check for banned C API usage' command: ci/banned.h.sh + - run: + name: 'Check for tests missing in schedules' + command: ci/check_all_tests_are_run.sh + - run: + name: 'Check if all CI scripts are actually run' + command: ci/check_all_ci_scripts_are_run.sh check-sql-snapshots: docker: - image: 'citus/extbuilder:latest' diff --git a/ci/README.md b/ci/README.md index f9afb7bd2..078c6e6d4 100644 --- a/ci/README.md +++ b/ci/README.md @@ -114,6 +114,22 @@ means that `latest.sql` is not up to date with the SQL file of the highest version number in the directory. The output of the script shows you what is different. +## `check_all_tests_are_run.sh` + +A test should always be included in a schedule file, otherwise it will not be +run in CI. This is most commonly forgotten for newly added tests. In that case +the dev ran it locally without running a full schedule with something like: +```bash +make -C src/test/regress/ check-minimal EXTRA_TESTS='multi_create_table_new_features' +``` + +## `check_all_ci_scripts_are_run.sh` + +This is the meta CI script. This checks that all existing CI scripts are +actually run in CI. This is most commonly forgotten for newly added CI tests +that the developer only ran locally. It also checks that all CI scripts have a +section in this `README.md` file and that they include `ci/ci_helpers.sh`. + ## `disallow_c_comments_in_migrations.sh` We do not use C-style comments in migration files as the stripped diff --git a/ci/check_all_ci_scripts_are_run.sh b/ci/check_all_ci_scripts_are_run.sh new file mode 100755 index 000000000..0b7abb3e3 --- /dev/null +++ b/ci/check_all_ci_scripts_are_run.sh @@ -0,0 +1,29 @@ +#!/bin/bash +set -euo pipefail + +# shellcheck disable=SC1091 +source ci/ci_helpers.sh + + +# 1. Find all *.sh files in the ci directory +# 2. Strip the directory +# 3. Exclude some scripts that we should not run in CI directly +ci_scripts=$( + find ci/ -iname "*.sh" | + sed -E 's#^ci/##g' | + grep -v -E '^(ci_helpers.sh|fix_style.sh)$' +) +for script in $ci_scripts; do + if ! grep "\\bci/$script\\b" .circleci/config.yml > /dev/null; then + echo "ERROR: CI script with name \"$script\" is not actually used in .circleci/config.yml" + exit 1 + fi + if ! grep "^## \`$script\`\$" ci/README.md > /dev/null; then + echo "ERROR: CI script with name \"$script\" does not have a section in ci/README.md" + exit 1 + fi + if ! grep "source ci/ci_helpers.sh" "ci/$script" > /dev/null; then + echo "ERROR: CI script with name \"$script\" does not include ci/ci_helpers.sh" + exit 1 + fi +done diff --git a/ci/check_all_tests_are_run.sh b/ci/check_all_tests_are_run.sh new file mode 100755 index 000000000..bc7568c08 --- /dev/null +++ b/ci/check_all_tests_are_run.sh @@ -0,0 +1,25 @@ +#!/bin/bash +set -euo pipefail + +# shellcheck disable=SC1091 +source ci/ci_helpers.sh + + +cd src/test/regress + +# 1. Find all *.sql *.spec and *.source files in the sql, spec and input +# directories +# 2. Strip the extension and the directory +# 3. Ignore names that end with .include, those files are meant to be in an C +# preprocessor #include statement. They should not be in schedules. +test_names=$( + find sql spec input -iname "*.sql" -o -iname "*.spec" -o -iname "*.source" | + sed -E 's#^\w+/([^/]+)\.[^.]+$#\1#g' | + grep -v '.include$' +) +for name in $test_names; do + if ! grep "\\b$name\\b" ./*_schedule > /dev/null; then + echo "ERROR: Test with name \"$name\" is not used in any of the schedule files" + exit 1 + fi +done diff --git a/src/test/regress/Makefile b/src/test/regress/Makefile index a0d13e95e..4f52b688c 100644 --- a/src/test/regress/Makefile +++ b/src/test/regress/Makefile @@ -111,6 +111,11 @@ check-base-mx: all $(pg_regress_multi_check) --load-extension=citus \ -- $(MULTI_REGRESS_OPTS) --schedule=$(citus_abs_srcdir)/mx_base_schedule $(EXTRA_TESTS) +check-minimal-mx: all + $(pg_regress_multi_check) --load-extension=citus \ + -- $(MULTI_REGRESS_OPTS) --schedule=$(citus_abs_srcdir)/mx_minimal_schedule $(EXTRA_TESTS) + + check-empty: all $(pg_regress_multi_check) --load-extension=citus \ -- $(MULTI_REGRESS_OPTS) $(EXTRA_TESTS) diff --git a/src/test/regress/expected/isolation_cancellation.out b/src/test/regress/expected/isolation_cancellation.out index 1947bd394..d82d7f264 100644 --- a/src/test/regress/expected/isolation_cancellation.out +++ b/src/test/regress/expected/isolation_cancellation.out @@ -12,7 +12,6 @@ step s1-reset: RESET ALL; step s1-drop: - DROP TABLE cancel_table; @@ -28,7 +27,6 @@ step s1-reset: RESET ALL; step s2-drop: - DROP TABLE cancel_table; @@ -50,7 +48,6 @@ step s1-reset: RESET ALL; step s1-drop: - DROP TABLE cancel_table; @@ -72,7 +69,6 @@ step s1-reset: RESET ALL; step s2-drop: - DROP TABLE cancel_table; @@ -97,7 +93,6 @@ step s1-reset: RESET ALL; step s1-drop: - DROP TABLE cancel_table; @@ -122,6 +117,5 @@ step s1-reset: RESET ALL; step s2-drop: - DROP TABLE cancel_table; diff --git a/src/test/regress/expected/multi_create_table_new_features.out b/src/test/regress/expected/multi_create_table_new_features.out index c3f707787..a3a52a6d2 100644 --- a/src/test/regress/expected/multi_create_table_new_features.out +++ b/src/test/regress/expected/multi_create_table_new_features.out @@ -6,13 +6,13 @@ CREATE TABLE table_identity_col ( id integer GENERATED BY DEFAULT AS IDENTITY PRIMARY KEY, payload text ); -SELECT master_create_distributed_table('table_identity_col', 'id', 'append'); +SELECT create_distributed_table('table_identity_col', 'id', 'append'); ERROR: cannot distribute relation: table_identity_col DETAIL: Distributed relations must not use GENERATED ... AS IDENTITY. SELECT create_distributed_table('table_identity_col', 'id'); ERROR: cannot distribute relation: table_identity_col DETAIL: Distributed relations must not use GENERATED ... AS IDENTITY. -SELECT create_distributed_table('table_identity_col', 'text'); +SELECT create_distributed_table('table_identity_col', 'payload'); ERROR: cannot distribute relation: table_identity_col DETAIL: Distributed relations must not use GENERATED ... AS IDENTITY. SELECT create_reference_table('table_identity_col'); diff --git a/src/test/regress/isolation_schedule b/src/test/regress/isolation_schedule index eb06be1c3..9d9c9a224 100644 --- a/src/test/regress/isolation_schedule +++ b/src/test/regress/isolation_schedule @@ -62,6 +62,7 @@ test: isolation_insert_select_conflict test: isolation_ref2ref_foreign_keys test: isolation_multiuser_locking test: shared_connection_waits +test: isolation_cancellation # MX tests test: isolation_reference_on_mx diff --git a/src/test/regress/multi_schedule b/src/test/regress/multi_schedule index d9dc2c4b2..33437e97b 100644 --- a/src/test/regress/multi_schedule +++ b/src/test/regress/multi_schedule @@ -46,7 +46,7 @@ test: multi_behavioral_analytics_basics multi_behavioral_analytics_single_shard_ test: multi_shard_update_delete recursive_dml_with_different_planners_executors test: insert_select_repartition window_functions dml_recursive multi_insert_select_window test: multi_insert_select_conflict create_table_triggers -test: multi_row_insert insert_select_into_local_table +test: multi_row_insert insert_select_into_local_table multi_create_table_new_features # following should not run in parallel because it relies on connection counts to workers test: insert_select_connection_leak diff --git a/src/test/regress/mx_minimal_schedule b/src/test/regress/mx_minimal_schedule new file mode 100644 index 000000000..c697f79dc --- /dev/null +++ b/src/test/regress/mx_minimal_schedule @@ -0,0 +1,9 @@ +# ---------- +# Only run few basic tests to set up a testing environment +# ---------- +test: multi_cluster_management +test: multi_test_helpers multi_test_helpers_superuser +test: multi_test_catalog_views + +# the following test has to be run sequentially +test: base_enable_mx diff --git a/src/test/regress/sql/multi_create_table_new_features.sql b/src/test/regress/sql/multi_create_table_new_features.sql index fea2ca990..e7f9cee39 100644 --- a/src/test/regress/sql/multi_create_table_new_features.sql +++ b/src/test/regress/sql/multi_create_table_new_features.sql @@ -12,6 +12,6 @@ CREATE TABLE table_identity_col ( SELECT create_distributed_table('table_identity_col', 'id', 'append'); SELECT create_distributed_table('table_identity_col', 'id'); -SELECT create_distributed_table('table_identity_col', 'text'); +SELECT create_distributed_table('table_identity_col', 'payload'); SELECT create_reference_table('table_identity_col');