Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request introduces parallel test execution infrastructure by refactoring the CI pipeline to partition tests across four concurrent environments, adding per-partition PostgreSQL containers, dynamic test port assignment via environment variables, failure aggregation, and a shell script orchestrator. Test configuration and ~80 test files are updated to enable async execution. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The diff introduces substantial CI/CD infrastructure changes with multi-stage test parallelization, dynamic environment configuration, and new failure aggregation logic. The Jenkinsfile refactor involves significant control flow reorganization across build, test, and push stages. While the ~80 async flag updates are homogeneous and repetitive, they require validation to ensure no test isolation issues arise from concurrent execution. The test framework configuration changes (dynamic ports, partition naming) and shell script orchestration add medium logic density. Overall heterogeneity stems from infrastructure, configuration, persistence, and mass test updates requiring separate reasoning for each category. Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
IvanIvanoff
force-pushed
the
improve-test-speed
branch
from
65303c9 to
b4aa644
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
test/sanbase_web/graphql/insight/timeline_event_comment_api_test.exs (1)
2-13:⚠️ Potential issue | 🔴 Critical
async: truewithclean_task_supervisor_children()creates a race condition that will cause flaky test failures.The macro terminates all children of the shared
Sanbase.TaskSupervisoron test exit. Withasync: true, when this test finishes andon_exitruns, concurrent test modules that have spawned tasks on the same supervisor will have those tasks killed mid-execution, causing intermittent failures.Either disable
async: truefor this test module or refactor the cleanup to use a test-local task supervisor instead of the shared global one.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanbase_web/graphql/insight/timeline_event_comment_api_test.exs` around lines 2 - 13, The test module currently uses async: true while calling clean_task_supervisor_children(), which kills children of the shared Sanbase.TaskSupervisor and causes race conditions; fix by disabling async for this module (change use SanbaseWeb.ConnCase, async: true to async: false) or refactor to create and use a test-local TaskSupervisor for this module (start a local supervisor in setup and replace calls to clean_task_supervisor_children() with terminating only that local supervisor), updating the setup block and cleanup to reference the new local supervisor rather than the global Sanbase.TaskSupervisor.test/sanbase_web/graphql/clickhouse/api_call_data_api_test.exs (1)
279-287:⚠️ Potential issue | 🟡 MinorCasing unification is no longer being tested.
The comment on Line 280 states that one query intentionally used
get_metricand the othergetMetricto verify casing unification. After this change, both helpers usegetMetric, so the snake_case → camelCase normalization path is no longer exercised. Either restore the originalget_metriccasing in this helper or remove the now-stale comment.Suggested fix — restore the original intent
defp get_metric(conn, metric, slug, from, to, interval) do # Intentionally one query is defined as get_metric and the other as getMetric # so casing unification is tested query = """ { - getMetric(metric: "#{metric}") { + get_metric(metric: "#{metric}") { timeseriesDataJson(slug: "#{slug}", from: "#{from}", to: "#{to}", interval: "#{interval}") } } """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanbase_web/graphql/clickhouse/api_call_data_api_test.exs` around lines 279 - 287, The comment says this helper should exercise snake_case→camelCase normalization but the GraphQL query currently uses "getMetric" in the get_metric helper; change the query string inside the get_metric function so it calls "get_metric(metric: ...)" (snake_case) instead of "getMetric", thereby restoring the original casing-unification test (or if you prefer to drop the test, remove the stale comment instead); ensure you update the query variable in the get_metric helper to the snake_case "get_metric" to re-enable the normalization path.test/sanbase_web/graphql/access_restrictions_test.exs (1)
28-46:⚠️ Potential issue | 🟠 MajorRemove
async: truefrom this test module or add cleanup for global state mutations.This test calls
Sanbase.Metric.Registry.refresh_stored_terms()(line 31), which writes to:persistent_term— global VM state shared across all processes. Whileasync: trueenables concurrent execution via Ecto database sandbox isolation, the sandbox does NOT isolate:persistent_term. Other concurrently running tests that read from the metric registry will observe this mutation, causing nondeterministic failures.Either keep the module as
async: false, or restore the original metric state via anon_exitcallback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanbase_web/graphql/access_restrictions_test.exs` around lines 28 - 46, The test mutates global VM state via Sanbase.Metric.Registry.refresh_stored_terms() (it writes to :persistent_term), so remove async: true from the test module declaration or add a setup/on_exit that captures the current persistent_term value before calling Sanbase.Metric.Registry.refresh_stored_terms() and restores it after the test (use :persistent_term.get/put or the registry's read/write helpers); locate the mutation in the test that calls refresh_stored_terms and the module attribute that sets async to change or add the setup/on_exit around this test (also check get_access_restrictions_for_metrics usage if it relies on the global registry state).
🧹 Nitpick comments (4)
test/sanbase_web/graphql/user/user_eth_account_api_test.exs (1)
2-2: Migrate from Tesla.Mock to Mox for external dependency mocking.
Tesla.Mockis deprecated and the codebase guidelines recommend usingMoxfor mocking external dependencies in test files (see coding guidelines fortest/**/*.exs). Replaceimport Tesla.Mockandmock(fn ...)calls with Mox allowances to align with project standards and future compatibility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanbase_web/graphql/user/user_eth_account_api_test.exs` at line 2, Tests currently import Tesla.Mock and call mock(fn ...) which should be replaced with Mox-based allowances; remove any import Tesla.Mock and mock(...) usage and instead configure Mox (add setup :verify_on_exit! in the test module), define or use the mock via Mox.defmock in your test helper, and replace mock(fn ...) stubs with Mox.allow/3 or Mox.expect/4 calls against the external HTTP client module (e.g., ExternalHttpClient or EthereumAPIClient used by the code under test) to return the same responses; ensure the mocked module is referenced in the app config for tests so the code under test calls the Mox mock.test/sanbase_web/views/error_view_test.exs (1)
5-5: Pre-existing:import Phoenix.Viewis deprecated.
Phoenix.Viewis no longer included with modern Phoenix. This isn't part of the current change, but worth noting for a future cleanup — the tests should usePhoenix.Template.render_to_string/3or the equivalent replacement. As per coding guidelines: "Phoenix.View no longer is needed or included with Phoenix. Do not use it."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanbase_web/views/error_view_test.exs` at line 5, Tests currently import the deprecated `Phoenix.View`; remove the `import Phoenix.View` line and update test calls that relied on that import to use `Phoenix.Template.render_to_string/3` (or the test helper equivalent) directly — e.g., replace any direct `render_to_string(...)` calls that depended on the import with `Phoenix.Template.render_to_string(ErrorView, "template.html", assigns)` or the appropriate view/template and assigns; ensure all uses reference the fully-qualified `Phoenix.Template.render_to_string/3` (or the app's test render helper) and run the tests to confirm no remaining `Phoenix.View` usage.lib/mix/failure_test_formatter.ex (1)
5-7: Missing@docand@specon the publicfailures_dir/0function.Per coding guidelines, public functions should have
@docdocumentation and@spectypespecs. This is minor given it's test infrastructure, but worth noting.Proposed addition
`@failures_dir` "_build/test/failures" + `@doc` "Returns the path to the directory where per-partition failure files are written." + `@spec` failures_dir() :: String.t() def failures_dir, do: `@failures_dir`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/mix/failure_test_formatter.ex` around lines 5 - 7, Add `@doc` and `@spec` for the public failures_dir/0 function: document what the function returns (the directory path used to store test failure artifacts) with a short `@doc` string and add a `@spec` failures_dir() :: String.t() so callers and dialyzer have a concrete contract; update the module around the failures_dir/0 definition to include these annotations directly above def failures_dir, referencing the existing `@failures_dir` and failures_dir/0.test_parallel.sh (1)
11-16: Database setup is sequential — consider parallelizing if setup time becomes a bottleneck.Each partition's DB setup (create, load, seed) runs serially. For 4 partitions this is likely fine, but if partition count grows or seed time is significant, these could be backgrounded similarly to the test runs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test_parallel.sh` around lines 11 - 16, The setup loops over PARTITIONS serially (commands mix ecto.create, mix ecto.load, mix run test/test_seeds.exs) which slows CI; run each partition's full setup in the background and then wait for all to finish. Wrap the three commands for a given MIX_TEST_PARTITION in a subshell or grouped block and background it (e.g., (MIX_TEST_PARTITION=$i mix ecto.create ...; MIX_TEST_PARTITION=$i mix ecto.load ...; MIX_TEST_PARTITION=$i mix run test/test_seeds.exs) &), collect PIDs and use wait to ensure all setups complete before proceeding, and propagate non-zero exits by checking wait statuses so failures aren’t silently ignored.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Jenkinsfile`:
- Line 23: The image tag creation uses env.CHANGE_ID which is null on non-PR
builds; update the Jenkinsfile so the imageTag construction uses a safe fallback
(e.g., compute a local changeId = env.CHANGE_ID ?: env.BRANCH_NAME ?: '' or
another appropriate fallback) and then build imageTag using that fallback
instead of env.CHANGE_ID directly (refer to the imageTag variable and
env.CHANGE_ID in the Jenkinsfile).
In `@lib/mix/failure_test_formatter.ex`:
- Around line 68-84: The partition file is written using the raw test_id which
for exit failures can include a trailing " (reason)" suffix (e.g.,
"test/foo.exs:42 (timeout)"), breaking shell re-run commands; update
write_failures_to_file to normalize the id before writing by stripping any
trailing " (…)" reason from the test id (or emit two fields like
"test_id\treason"), e.g. in write_failures_to_file iterate the test_id values
and replace/remove a terminal space-plus-parentheses suffix (pattern like
/\s+\(.*\)$/) so only "file:line" is written alongside kind, ensuring
test_parallel.sh receives valid test selectors.
In `@test_parallel.sh`:
- Around line 79-80: The arrays all_tests, error_tests, exit_tests, and
invalid_tests can trigger "unbound variable" under set -u when some arrays are
empty; update the construction of all_tests and the echo to use safe expansions
(e.g., use "${exit_tests[@]:-}" and "${invalid_tests[@]:-}" or conditionally
append only non-empty arrays) so empty arrays don’t cause an unbound-variable
error; adjust any references to "${error_tests[@]}", "${exit_tests[@]}", and
"${invalid_tests[@]}" in the test assembly and the echo line accordingly.
In `@test/sanbase_web/graphql/chart/chart_configuration_api_test.exs`:
- Line 2: The test does an exact-equality check on the global public chart
configurations which is flaky under async tests; update the assertion in
chart_configuration_api_test (the test that builds config_id1 and reads
config_ids) to check membership instead of equality—replace the equality
assertion (assert config_ids == [config_id1]) with a membership assertion
(assert config_id1 in config_ids) so the test only requires that the created
config is present among public configs.
In `@test/sanbase_web/graphql/moderation/entity_moderation_api_test.exs`:
- Line 2: The test file sets async: true on the ConnCase which causes per-test
clears of the global SanbaseWeb.Graphql.Cache (see calls to
Sanbase.Cache.clear_all/0 and direct clears of :sanbase_cache/:graphql_cache) to
race with other tests; remove async: true from the use SanbaseWeb.ConnCase line
or refactor the tests to use a namespaced, test-specific cache instance (replace
global :sanbase_cache/:graphql_cache usage with a test-scoped cache name and
ensure creation/teardown there) and delete the redundant
Sanbase.Cache.clear_all() call that duplicates ConnCase.setup behavior; update
references in the file to the cache-clearing calls and any setup helpers (e.g.,
Graphql.Cache clearing sites) accordingly so tests run deterministically.
In `@test/sanbase_web/graphql/watchlist/project/watchlist_api_test.exs`:
- Line 2: The cleanup hook clean_task_supervisor_children() is unsafe for async
tests because it kills all children of the shared Sanbase.TaskSupervisor;
refactor tests to use a per-test supervisor instead of a global one: modify test
setup to start a DynamicSupervisor (or Task.Supervisor) with a unique name or
via process registration scoped to the test (e.g., start_supervisor_for_test/0
in the test setup) and ensure all tasks in the tests are started under that
supervisor (replace calls that use Sanbase.TaskSupervisor with the per-test
supervisor variable); remove the global on_exit/1 that terminates
Sanbase.TaskSupervisor children, or scope its on_exit to only stop the per-test
supervisor; if per-test isolation cannot be implemented, set async: false on the
ConnCase usage to avoid race conditions, and ensure any assert_receive usage
waits for tasks started under the per-test supervisor.
In `@test/sanbase/ai/openai_client_test.exs`:
- Around line 2-3: Remove the unnecessary ConnCase setup by deleting the `use
SanbaseWeb.ConnCase` line in the test file and leave only `use ExUnit.Case,
async: true`; the test uses Mox only and does not require ConnCase features (no
conn, `@endpoint`, DB sandbox or seeding). Verify no ConnCase helpers are
referenced in the file (e.g., ensure the module using `use ExUnit.Case` still
runs any mocks) and run the tests to confirm removal didn't break setup.
In `@test/sanbase/external_services/coinmarketcap/ticker_fetcher_test.exs`:
- Line 2: The test suffers from global state and named-process collisions: add
Sanbase.InMemoryKafka.Producer.clear_state() to the test setup to reset the
Agent before each test, and change the KafkaExporter.start_link(name:
:prices_exporter, ...) invocation to handle already-started errors (e.g., rescue
or case-match on {:error, {:already_started, _}}) or make the test synchronous
by removing async: true; target the KafkaExporter.start_link call and
Sanbase.InMemoryKafka.Producer.get_state()/clear_state() to implement these
fixes.
In `@test/sanbase/external_services/coinmarketcap/web_api_test.exs`:
- Line 2: The test module is marked async via the "use Sanbase.DataCase, async:
true" invocation which causes race conditions with the globally-named Agent in
Sanbase.InMemoryKafka.Producer; change that invocation to "use Sanbase.DataCase,
async: false" so tests run synchronously and the shared Agent state is not
concurrently cleared/modified by multiple test processes.
In `@test/sanbase/kafka/kafka_exporter_test.exs`:
- Line 2: The test suite enables async via use ExUnit.Case, async: true and the
"trigger multiple batch sends" test uses Process.sleep(300) to wait for
InMemoryKafka.Producer to receive 10_000 messages, which can be flaky; either
disable async for this test module (remove or set async: false) or increase the
sleep margin (e.g., to 1000–2000 ms) and/or replace the timing-based wait with a
deterministic wait (poll/assert until InMemoryKafka.Producer reports the
expected count). Also avoid creating dynamic atoms with
String.to_atom(random_bytes); change that to a binary-based topic name or use
String.to_existing_atom/other safe approach to prevent atom table pollution.
---
Outside diff comments:
In `@test/sanbase_web/graphql/access_restrictions_test.exs`:
- Around line 28-46: The test mutates global VM state via
Sanbase.Metric.Registry.refresh_stored_terms() (it writes to :persistent_term),
so remove async: true from the test module declaration or add a setup/on_exit
that captures the current persistent_term value before calling
Sanbase.Metric.Registry.refresh_stored_terms() and restores it after the test
(use :persistent_term.get/put or the registry's read/write helpers); locate the
mutation in the test that calls refresh_stored_terms and the module attribute
that sets async to change or add the setup/on_exit around this test (also check
get_access_restrictions_for_metrics usage if it relies on the global registry
state).
In `@test/sanbase_web/graphql/clickhouse/api_call_data_api_test.exs`:
- Around line 279-287: The comment says this helper should exercise
snake_case→camelCase normalization but the GraphQL query currently uses
"getMetric" in the get_metric helper; change the query string inside the
get_metric function so it calls "get_metric(metric: ...)" (snake_case) instead
of "getMetric", thereby restoring the original casing-unification test (or if
you prefer to drop the test, remove the stale comment instead); ensure you
update the query variable in the get_metric helper to the snake_case
"get_metric" to re-enable the normalization path.
In `@test/sanbase_web/graphql/insight/timeline_event_comment_api_test.exs`:
- Around line 2-13: The test module currently uses async: true while calling
clean_task_supervisor_children(), which kills children of the shared
Sanbase.TaskSupervisor and causes race conditions; fix by disabling async for
this module (change use SanbaseWeb.ConnCase, async: true to async: false) or
refactor to create and use a test-local TaskSupervisor for this module (start a
local supervisor in setup and replace calls to clean_task_supervisor_children()
with terminating only that local supervisor), updating the setup block and
cleanup to reference the new local supervisor rather than the global
Sanbase.TaskSupervisor.
---
Nitpick comments:
In `@lib/mix/failure_test_formatter.ex`:
- Around line 5-7: Add `@doc` and `@spec` for the public failures_dir/0 function:
document what the function returns (the directory path used to store test
failure artifacts) with a short `@doc` string and add a `@spec` failures_dir() ::
String.t() so callers and dialyzer have a concrete contract; update the module
around the failures_dir/0 definition to include these annotations directly above
def failures_dir, referencing the existing `@failures_dir` and failures_dir/0.
In `@test_parallel.sh`:
- Around line 11-16: The setup loops over PARTITIONS serially (commands mix
ecto.create, mix ecto.load, mix run test/test_seeds.exs) which slows CI; run
each partition's full setup in the background and then wait for all to finish.
Wrap the three commands for a given MIX_TEST_PARTITION in a subshell or grouped
block and background it (e.g., (MIX_TEST_PARTITION=$i mix ecto.create ...;
MIX_TEST_PARTITION=$i mix ecto.load ...; MIX_TEST_PARTITION=$i mix run
test/test_seeds.exs) &), collect PIDs and use wait to ensure all setups complete
before proceeding, and propagate non-zero exits by checking wait statuses so
failures aren’t silently ignored.
In `@test/sanbase_web/graphql/user/user_eth_account_api_test.exs`:
- Line 2: Tests currently import Tesla.Mock and call mock(fn ...) which should
be replaced with Mox-based allowances; remove any import Tesla.Mock and
mock(...) usage and instead configure Mox (add setup :verify_on_exit! in the
test module), define or use the mock via Mox.defmock in your test helper, and
replace mock(fn ...) stubs with Mox.allow/3 or Mox.expect/4 calls against the
external HTTP client module (e.g., ExternalHttpClient or EthereumAPIClient used
by the code under test) to return the same responses; ensure the mocked module
is referenced in the app config for tests so the code under test calls the Mox
mock.
In `@test/sanbase_web/views/error_view_test.exs`:
- Line 5: Tests currently import the deprecated `Phoenix.View`; remove the
`import Phoenix.View` line and update test calls that relied on that import to
use `Phoenix.Template.render_to_string/3` (or the test helper equivalent)
directly — e.g., replace any direct `render_to_string(...)` calls that depended
on the import with `Phoenix.Template.render_to_string(ErrorView,
"template.html", assigns)` or the appropriate view/template and assigns; ensure
all uses reference the fully-qualified `Phoenix.Template.render_to_string/3` (or
the app's test render helper) and run the tests to confirm no remaining
`Phoenix.View` usage.
test/sanbase_web/graphql/moderation/entity_moderation_api_test.exs
Outdated
Show resolved
Hide resolved
test/sanbase/external_services/coinmarketcap/ticker_fetcher_test.exs
Outdated
Show resolved
Hide resolved
IvanIvanoff
force-pushed
the
improve-test-speed
branch
from
b4aa644 to
5e5c8b3
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/sanbase/insight/categorizer_test.exs (1)
2-10:⚠️ Potential issue | 🟡 MinorRemove unused Mox import and
verify_on_exit!setup.This test module imports Mox and calls
setup :verify_on_exit!, but the tests don't use Mox for mocking at all. They rely on ExMachina factories and direct database operations. Remove the unnecessaryimport Moxandsetup :verify_on_exit!lines—they add no value and can cause confusion about the test's purpose.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/sanbase/insight/categorizer_test.exs` around lines 2 - 10, Remove the unused Mox integration by deleting the line "import Mox" and the test setup invocation "setup :verify_on_exit!" from the test module; ensure you only remove those two lines (leave use Sanbase.DataCase, import Sanbase.Factory, and the existing alias declarations like Sanbase.Insight.{Category, PostCategory, Categorizer} and Sanbase.Repo intact).
🧹 Nitpick comments (1)
Jenkinsfile (1)
60-70: Consider adding--max-failuresor a timeout to prevent runaway test containers.Each partition runs
mix testinside a Docker container with no time limit. If a test hangs indefinitely, the CI job will block until the Jenkins timeout kills it, leaving orphaned Postgres containers (though thefinallyblock should catch those). Consider adding--timeouttodocker runor--max-failurestomix testto fail fast.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Jenkinsfile` around lines 60 - 70, The docker-run block that executes the mix test command (the invocation using imageTag, failuresDir, buildSuffix, partition and numPartitions) has no time limit and can hang; update the pipeline to fail fast by adding an ExUnit max-failures flag (add --max-failures N to the mix test invocation) and/or ensure the docker run is bounded (wrap the docker run with a timeout wrapper or configure a docker stop timeout) so hung tests are killed and cleanup runs; modify the mix test invocation inside the docker run (the line that calls "mix test --partitions ${numPartitions} ...") to include --max-failures and/or adjust how docker run is launched to enforce a timeout.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Jenkinsfile`:
- Around line 87-103: The shell heredoc in the Jenkinsfile "Summarize Test
Failures" block is using bare single-quoted strings (e.g., '',
'========================================', 'Failing tests across all
partitions:') which the shell tries to execute; change those to proper output
commands (e.g., echo or printf) so they print instead of being run as commands.
Update the shell script that references failuresDir and partition_*.txt (the
heredoc containing the ls/cat/cut/sort pipeline and rm -rf ${failuresDir}) to
prefix those banner and message lines with echo/printf (or wrap the whole
heredoc in cat <<'EOF' ... EOF) so the banners, "Failing tests across all
partitions:", "Re-run with:", and the empty lines are printed correctly. Ensure
any embedded command substitutions like "$(cat ${failuresDir}/partition_*.txt |
cut -f2 ...)" remain evaluated as intended.
- Around line 46-52: The loop that starts Postgres containers (for (int i = 1; i
<= numPartitions; i++) { sh "docker run ... -d pgvector/pgvector:pg15" })
launches containers but doesn't wait for DB readiness; add a readiness step
after each container start: either run docker run with a
--health-cmd/--health-interval/--health-retries config and then poll docker
inspect --format '{{.State.Health.Status}}' until "healthy", or after starting
each container use a short wait loop that executes pg_isready (or docker exec
... pg_isready) against test-postgres-${buildSuffix}-p${i} and only proceed when
it reports ready (with a timeout and clear error logging). Ensure this logic
references the same buildSuffix and container name pattern used in the loop.
- Around line 83-104: The 'Summarize Test Failures' stage reads files created
inside the docker container but is not executed within container('docker'),
causing pwd()/failuresDir to resolve in the wrong context; update the pipeline
so the stage 'Summarize Test Failures' runs inside container('docker') (e.g.,
wrap the stage body or its steps in container('docker') so the failuresDir
computed with pwd() points to the same container filesystem where
partition_*.txt was created).
In `@test_parallel.sh`:
- Around line 26-33: The pipeline in the loop is backgrounded directly so $!
refers to sed's PID and wait captures sed's (successful) exit code, hiding test
failures; wrap the whole pipeline in a subshell that sets "set -o pipefail" (or
"set -euo pipefail" if desired) so the subshell exits with the pipeline's real
status, background that subshell (so $! is the subshell PID) and store those
PIDs (pids+=($!)); then use wait on those stored PIDs to correctly propagate mix
test failures (refer to the for loop that sets MIX_TEST_PARTITION and runs mix
test with sed).
---
Outside diff comments:
In `@test/sanbase/insight/categorizer_test.exs`:
- Around line 2-10: Remove the unused Mox integration by deleting the line
"import Mox" and the test setup invocation "setup :verify_on_exit!" from the
test module; ensure you only remove those two lines (leave use Sanbase.DataCase,
import Sanbase.Factory, and the existing alias declarations like
Sanbase.Insight.{Category, PostCategory, Categorizer} and Sanbase.Repo intact).
---
Duplicate comments:
In `@Jenkinsfile`:
- Line 23: The image tag construction uses env.CHANGE_ID which is null on non-PR
builds, causing "null" in the tag; update the Jenkinsfile where imageTag is
defined (refer to imageTag, scmVars.GIT_COMMIT, env.BUILD_ID,
sanitizedBranchName, env.CHANGE_ID) to substitute a safe default or omit
CHANGE_ID when it's unset (e.g., use a null-coalescing/ternary expression so the
tag uses a fallback like "none" or an empty segment instead of "null").
In `@test_parallel.sh`:
- Line 86: Under set -u an empty array expansion can trigger "unbound variable";
update the all_tests assignment to safely expand possibly-empty arrays by using
parameter expansion with a default for each array (e.g. use
"${error_tests[@]:-}", "${exit_tests[@]:-}", "${invalid_tests[@]:-}" in the
all_tests=(...) expression) so empty arrays don't cause an unbound variable
error when referenced.
In `@test/sanbase_web/graphql/moderation/entity_moderation_api_test.exs`:
- Line 2: The test module currently enables concurrent runs via "use
SanbaseWeb.ConnCase, async: true" but calls global cache clearers
(Sanbase.Cache.clear_all() in setup and SanbaseWeb.Graphql.Cache.clear_all()
throughout tests), which is unsafe; change the ConnCase invocation to disable
async by replacing async: true with async: false (or, alternatively, refactor
tests to avoid global cache mutations by scoping/isolating cache use and
removing calls to Sanbase.Cache.clear_all and
SanbaseWeb.Graphql.Cache.clear_all), ensuring the module-level symbol "use
SanbaseWeb.ConnCase" is updated accordingly.
In `@test/sanbase/ai/openai_client_test.exs`:
- Around line 2-3: Remove the unnecessary ConnCase setup from the test module:
delete the `use SanbaseWeb.ConnCase` invocation so the test only `use
ExUnit.Case, async: true`; ensure Mox remains available (import/alias Mox or
call Mox.setup/expectations elsewhere if your test relies on it) and that no
ConnCase-specific setup (sandbox/endpoint) is expected by functions in this file
such as those asserting mocks for OpenAI client.
In `@test/sanbase/external_services/coinmarketcap/ticker_fetcher_test.exs`:
- Line 2: The test is unsafe to run async; change "use Sanbase.DataCase, async:
true" to "async: false" (or remove async) to prevent global-state collisions, or
if you prefer to keep async then (1) stop registering a global GenServer name by
making KafkaExporter.start_link use a unique name per test (avoid
:prices_exporter), (2) replace the single-arity Tesla.Mock.mock/1 global mock
with a test-scoped mock (use the two-argument form or Tesla.Mock.mock_global/1
with proper isolation), and (3) ensure the InMemoryKafka Agent state is isolated
per test by clearing or using unique topic/state names before each test instead
of relying on the global "asset_prices" state; reference
KafkaExporter.start_link, Tesla.Mock.mock/1, and
InMemoryKafka.Producer.get_state for changes.
In `@test/sanbase/kafka/kafka_exporter_test.exs`:
- Line 142: rand_atom/0 currently converts random bytes into new atoms via
String.to_atom, which leaks the atom table; change rand_atom to avoid creating
arbitrary atoms by either returning a string name (e.g., Base.encode64(... ) as
a binary) or produce a safe unique atom using an existing-interpolated literal
like :"test_kafka_#{System.unique_integer()}" so tests don't continuously
allocate new atoms; update any callers that expect an atom to accept the string
or the new unique atom returned by rand_atom.
- Line 2: The test uses async: true and Process.sleep-based waits against a
shared InMemoryKafka.Producer which can cause flakiness; change the test module
declaration in kafka_exporter_test.exs from use ExUnit.Case, async: true to use
ExUnit.Case, async: false to serialize access, or alternatively remove
Process.sleep calls and replace them with polling/retry assertions that check
InMemoryKafka.Producer state (e.g., loop with a short sleep and a timeout and
assert when the expected message count/condition is observed) so tests no longer
rely on fixed sleeps.
---
Nitpick comments:
In `@Jenkinsfile`:
- Around line 60-70: The docker-run block that executes the mix test command
(the invocation using imageTag, failuresDir, buildSuffix, partition and
numPartitions) has no time limit and can hang; update the pipeline to fail fast
by adding an ExUnit max-failures flag (add --max-failures N to the mix test
invocation) and/or ensure the docker run is bounded (wrap the docker run with a
timeout wrapper or configure a docker stop timeout) so hung tests are killed and
cleanup runs; modify the mix test invocation inside the docker run (the line
that calls "mix test --partitions ${numPartitions} ...") to include
--max-failures and/or adjust how docker run is launched to enforce a timeout.
8fbce76 to
5e5c8b3
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Jenkinsfile (1)
38-82: Test image is never cleaned up after use.The image tagged at Line 27 (
sanbase-test:...) is built per-run with a unique tag but never removed. Over time these will accumulate on the build agent and consume disk. Consider addingdocker rmi ${imageTag} || truein apostblock or after the test stages.Suggested cleanup addition (e.g., after the Summarize stage)
+ stage('Cleanup') { + container('docker') { + sh "docker rmi ${env.TEST_IMAGE_TAG} || true" + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Jenkinsfile` around lines 38 - 82, The test image created with the TEST_IMAGE_TAG stored in the imageTag variable is never removed, causing disk buildup; add a cleanup step that runs docker rmi ${imageTag} || true after the parallel test execution (for example in the finally block inside the "Run Tests" stage or in a pipeline post block for that stage) so the image built per run is removed; reference imageTag, buildSuffix, and the "Run Tests" stage when adding this cleanup and ensure it runs even on failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Jenkinsfile`:
- Around line 47-53: After starting each Postgres container in the Jenkinsfile
loop (containers named test-postgres-${buildSuffix}-p${i}), add a readiness
polling loop that waits until the DB accepts connections (e.g., run docker exec
test-postgres-${buildSuffix}-p${i} pg_isready -U postgres or attempt a simple
psql connection) with a short sleep between attempts and a reasonable overall
timeout; if the timeout is reached, fail the build with a clear error. Place
this loop immediately after each docker run in the for loop that iterates over
numPartitions so tests only start once every test-postgres-${buildSuffix}-p${i}
is ready.
- Around line 84-105: The "Summarize Test Failures" stage currently runs outside
container('docker') causing pwd() and the sh step to potentially reference a
different filesystem; wrap the entire stage (stage('Summarize Test Failures'))
body in container('docker') so the failuresDir computation and the sh block run
inside the same docker workspace where partition_*.txt files were created,
ensuring failuresDir and the rm -rf operate against the correct path.
---
Nitpick comments:
In `@Jenkinsfile`:
- Around line 38-82: The test image created with the TEST_IMAGE_TAG stored in
the imageTag variable is never removed, causing disk buildup; add a cleanup step
that runs docker rmi ${imageTag} || true after the parallel test execution (for
example in the finally block inside the "Run Tests" stage or in a pipeline post
block for that stage) so the image built per run is removed; reference imageTag,
buildSuffix, and the "Run Tests" stage when adding this cleanup and ensure it
runs even on failures.
c6ec58d to
b0f6ff8
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/mix/failure_test_formatter.ex (1)
25-28: Remove dead code:_reasonis computed but never used.The variable is assigned but discarded. The comment explains intent, but keeping dead computation is unnecessary noise. Just delete lines 25-26.
Proposed cleanup
- # Do not add the reason for now - _reason = if kind == :exit and is_tuple(error), do: " (#{elem(error, 0)})", else: "" - test_identifier = "#{file}:#{line}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/mix/failure_test_formatter.ex` around lines 25 - 28, Remove the dead computation of _reason (the conditional assignment using kind and error) and its preceding comment; the variable is never used afterwards—locate the block where _reason is assigned (references: _reason, kind, error) and delete that assignment and the "Do not add the reason for now" comment, leaving test_identifier = "#{file}:#{line}" intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Jenkinsfile`:
- Around line 47-53: The loop that starts Postgres containers (for (int i = 1; i
<= numPartitions; i++) and the sh that runs docker run --name
test-postgres-${buildSuffix}-p${i}) must be followed by a readiness wait for
each container to avoid race conditions; after each container start, run a retry
loop that calls docker exec test-postgres-${buildSuffix}-p${i} pg_isready (or
docker run --rm --network ... pg_isready targeting the container) with a short
sleep and overall timeout, and fail the pipeline if pg_isready does not report
ready within the timeout so that subsequent mix test runs only start once the DB
is initialized.
---
Nitpick comments:
In `@lib/mix/failure_test_formatter.ex`:
- Around line 25-28: Remove the dead computation of _reason (the conditional
assignment using kind and error) and its preceding comment; the variable is
never used afterwards—locate the block where _reason is assigned (references:
_reason, kind, error) and delete that assignment and the "Do not add the reason
for now" comment, leaving test_identifier = "#{file}:#{line}" intact.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/sanbase_web/api_call_limit/api_call_limit_api_test.exs`:
- Around line 644-646: Fix the typo in the test comment: change
"quota_size_max_offse" to "quota_size_max_offset" so the comment reads
"quota_size + quota_size_max_offset"; the comment appears above the loop that
calls make_api_call in
test/sanbase_web/api_call_limit/api_call_limit_api_test.exs (refer to the string
"quota_size_max_offse" and the make_api_call(...) loop) — update only the
comment text.
- Around line 292-298: The comment describing the test timing is stale: update
or remove the sentences that reference "7 of the iterations" and "3 days before
the month ends" so they reflect the current variables (iterations = 3 and
days_in_old_month = 2) in
test/sanbase_web/api_call_limit/api_call_limit_api_test.exs; specifically edit
the comment around the variables iterations and days_in_old_month to accurately
describe why `now` is set relative to month end and how many iterations will run
given the new values (or simplify to a short comment stating the intent without
exact counts).
In `@test/sanbase/app_notifications/app_notifications_test.exs`:
- Around line 644-645: The drain timeout was reduced to 1_000ms which can cause
flaky CI tests; restore or increase the timeout used when calling
Sanbase.EventBus.drain_topics (the call that follows
Sanbase.EventBus.AppNotificationsSubscriber.topics()) to match the longer
10_000ms used in setup_all/on_exit so the three rapid vote events have enough
time to be processed before the deduplication assertion.
---
Duplicate comments:
In `@test_parallel.sh`:
- Around line 89-93: The concatenation of arrays all_tests from error_tests,
exit_tests, and invalid_tests will fail under set -u when any source array is
unset/empty; update the assignment that builds all_tests to use safe
parameter-expansions for each array (e.g., use the "${error_tests[@]:-}",
"${exit_tests[@]:-}", "${invalid_tests[@]:-}" pattern) so expanding empty or
unset arrays does not trigger an "unbound variable" error; keep the rest of the
script (printf ' mix test' and printf ' %q' "${all_tests[@]}") unchanged.
0664b01 to
357eb1a
Compare
IvanIvanoff
force-pushed
the
improve-test-speed
branch
from
357eb1a to
4886420
Compare
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
Ticket
Checklist:
Summary by CodeRabbit
New Features
Tests
Chores