Improve materialize_test macro for the disabled situation

[yu-iskw]

Solves #906 (comment)

Summary by CodeRabbit

Release Notes

• Improvements
  • Enhanced test result collection with more granular control conditions, allowing users to disable test results collection through configuration settings.
  • Refined test materialization logic to prevent collection during system initialization phases.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

📝 Walkthrough

Walkthrough

A new configuration macro is introduced to consolidate test result collection conditions, replacing the test materialization's direct dependency on is_elementary_enabled(). The updated macro evaluates execution phase, Elementary enablement, configuration, and CLI context before collecting test results.

Changes

Cohort / File(s)	Summary
New Configuration Macro macros/edr/system/configuration/should_collect_test_results.sql	Introduces should_collect_test_results() macro that evaluates four conditions via AND logic: active execution phase, Elementary enabled, test results not disabled via config, and not an EDR CLI initialization run. Returns computed boolean result.
Updated Test Materialization macros/edr/materializations/test/test.sql	Condition check changed from elementary.is_elementary_enabled() to elementary.should_collect_test_results() to determine early return behavior. No other logic altered.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 A new guardian macro hops into place,
Checking phases, configs, and traces,
No more just enabled—now wisdom's the way,
Four conditions aligned to collect results today! ✨

Pre-merge checks

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: improving the materialize_test macro to handle disabled situations by replacing the condition check from is_elementary_enabled() to should_collect_test_results().
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[github-actions]

👋 @yu-iskw
Thank you for raising your pull request.
Please make sure to add tests and document all user-facing changes.
You can do this by editing the docs files in the elementary repository.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

macros/edr/system/configuration/should_collect_test_results.sql (1)

11-11: Consider the readability of the double negative.

The expression not elementary.get_config_var('disable_tests_results') creates a double negative that could be harder to reason about. While logically correct, this pattern is slightly less intuitive than a positive configuration like 'enable_tests_results'.

This is optional since changing the config variable name would be a broader refactoring beyond this PR's scope.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between a45c384 and 772c849.

📒 Files selected for processing (2)

• macros/edr/materializations/test/test.sql
• macros/edr/system/configuration/should_collect_test_results.sql

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2025-09-04T09:14:40.621Z

Learnt from: haritamar
Repo: elementary-data/dbt-data-reliability PR: 825
File: macros/edr/tests/test_dimension_anomalies.sql:73-75
Timestamp: 2025-09-04T09:14:40.621Z
Learning: In the dbt-data-reliability codebase, `flatten_model` is a valid macro defined in `macros/edr/dbt_artifacts/upload_dbt_models.sql` and can be used as a parameter to `elementary.get_anomaly_query()` even though other test files use the `flattened_test` variable instead.

Applied to files:

• macros/edr/materializations/test/test.sql

🔇 Additional comments (3)

macros/edr/system/configuration/should_collect_test_results.sql (2)

1-16: Well-structured consolidation of collection conditions.

The macro correctly consolidates four conditions with clear documentation. The logic properly combines execution phase, enablement state, configuration, and CLI context checks.

---

10-14: No action needed. Both disable_tests_results and edr_cli_run have safe defaults (false) defined in the configuration, and get_config_var() is designed to fall back to these defaults if variables are undefined. The macro behavior is already predictable and graceful.

Likely an incorrect or invalid review comment.

macros/edr/materializations/test/test.sql (1)

17-17: Verify the behavioral impact of the stricter collection criteria.

The change correctly replaces the simple enablement check with a more comprehensive condition. The new should_collect_test_results() macro adds three additional conditions beyond the original is_elementary_enabled() check:

1. Requires execution phase (execute)
2. Checks tests_results_enabled config
3. Excludes EDR CLI runs

This means test results will be skipped in more scenarios than before. While this aligns with the PR objective to handle "disabled situations," confirm that existing workflows (particularly in compilation/parsing phases or EDR CLI contexts) are not negatively impacted.