Skip to content

chore(gooddata-eval): use neutral identifiers in fixtures, drop a hardcoded path - #1827

Merged
Tomkess merged 1 commit into
masterfrom
chore/tidy-eval-test-fixtures
Sep 24, 2026
Merged

Tomkess merged 1 commit into
masterfrom
chore/tidy-eval-test-fixtures

Conversation

@Tomkess

@Tomkess Tomkess commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Housekeeping in gooddata-eval, no behaviour change.

What

Fixtures use generic names. The scoring tests exercise attribute-filter ordering and ranking-filter equivalence; the metric and label names in them were copied out of one particular workspace and carry no meaning for a reader of this package. They are now ordinary names (label/region_name, metric/conversion_rate, demo_workspace).

verify_guardrail_refusal_criteria.py is runnable again. It loaded .env from an absolute path under one developer's home directory, so nobody else could run it. It now reads GD_EVAL_ENV_FILE, defaulting to .env in the working directory.

Two comments say what they mean instead of pointing at a repository the reader cannot open.

One thing worth a second look

test_the_same_elements_on_a_different_label_still_differ names two different labels on purpose — that difference is the whole assertion. Renaming both to the same value turns it green while testing nothing, which is exactly what happened on the first pass here. They stay distinct, and the test still fails if the comparison stops caring about the label.

Verification

1111 tests pass, ruff check and ruff format --check clean.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Developer Experience
    • Evaluation verification tooling can now load configuration from a selectable environment file, defaulting to .env, rather than relying on a fixed local path. This makes the tool easier to run across different development environments.
  • Other Changes
    • Updated evaluation examples and explanatory text. These changes do not affect evaluation behavior.

…dcoded path

Test fixtures carried metric and label names copied out of one particular
workspace, and two comments named an external repository rather than saying what
they meant. Neither tells a reader of this package anything: the scoring tests
exercise attribute-filter ordering and the names are arbitrary, so they now use
generic ones.

verify_guardrail_refusal_criteria.py loaded .env from an absolute path under one
developer's home directory, which made the script runnable on exactly one machine.
It now reads GD_EVAL_ENV_FILE, defaulting to .env in the working directory.

One test needed care rather than a rename: the two sides of
test_the_same_elements_on_a_different_label_still_differ deliberately name
different labels, and renaming both to the same thing quietly turned it into a
test of nothing. They stay distinct.

No behaviour change. 1111 tests pass.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: d762f5a5-5277-4a9e-a143-755ffb99660d

📥 Commits

Reviewing files that changed from the base of the PR and between 9927b28 and 4063cf5.

📒 Files selected for processing (4)
  • packages/gooddata-eval/scripts/verify_guardrail_refusal_criteria.py
  • packages/gooddata-eval/src/gooddata_eval/core/models.py
  • packages/gooddata-eval/tests/test_from_insights.py
  • packages/gooddata-eval/tests/test_scoring.py

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The guardrail script now loads dotenv from a configurable path. Evaluation comments and test fixtures also use updated descriptions, labels, and workspace values.

Changes

Guardrail script environment

Layer / File(s) Summary
Configurable dotenv path
packages/gooddata-eval/scripts/verify_guardrail_refusal_criteria.py
The script reads the dotenv path from GD_EVAL_ENV_FILE and defaults to .env, replacing a machine-specific absolute path.

Evaluation descriptions and fixtures

Layer / File(s) Summary
Scoring examples and labels
packages/gooddata-eval/src/gooddata_eval/core/models.py, packages/gooddata-eval/tests/test_scoring.py
The comment about reasoning-summary labels and the scoring test examples use updated descriptions and attribute labels. The tests retain their matching and mismatch assertions.
Dataset item workspace fixture
packages/gooddata-eval/tests/test_from_insights.py
The test now builds and expects a dataset item named demo_workspace.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~8 minutes

Change: Other

Suggested reviewers: lupko

Merge Risk: ⚪ Minimal · up to 4063c

The changes make the guardrail script’s environment-file path configurable and refresh evaluation examples without an apparent behavior-breaking mismatch. The PR is mergeable subject to normal checks.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 4 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main changes: replacing workspace-specific fixture identifiers with neutral identifiers and removing a hardcoded environment-file path.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR

A rabbit checks the dotenv trail,
Now .env can change its trail.
New labels hop through scoring tests,
A demo workspace joins the rest.
The rabbit nods: the updates pass.

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Sep 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.88%. Comparing base (9927b28) to head (4063cf5).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1827   +/-   ##
=======================================
  Coverage   82.88%   82.88%           
=======================================
  Files         328      328           
  Lines       21235    21235           
=======================================
  Hits        17600    17600           
  Misses       3635     3635           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Tomkess
Tomkess merged commit 782acee into master Sep 24, 2026
14 checks passed
@Tomkess
Tomkess deleted the chore/tidy-eval-test-fixtures branch September 24, 2026 11:57
Tomkess added a commit that referenced this pull request Sep 24, 2026
…l-open-prs

Master has moved on nine gooddata-eval commits, five of them PRs this branch
was carrying from their own branches (#1793, #1799, #1806, #1807, #1827) and
four the dashboard-skill stack.

Resolutions worth naming:

- Per-run failure capture vs the new timeline detail. Master replaced
  `build_latency_breakdown` with `timeline_detail` (latency breakdown plus the
  tool calls) at the four `detail` sites this branch had refactored into a
  shared `_run_detail`. Kept the refactor and added `timeline_detail` for the
  winning run on top, so failing runs keep the compact breakdown and the best
  run still reports the calls themselves.

- `dashboard_skill` arrived from master without per-run failure records, which
  the structural guard test requires of every multi-run kind. Wired it the same
  way as its siblings.

- Sort normalisation and the scrubbed fixture comments were taken from master
  wholesale; this branch predates both.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants