Skip to content

(feat): merging analysis tests#9872

Merged
eytan-starkware merged 1 commit intomainfrom
eytan_graphite/_feat_merging_analysis_tests
May 3, 2026
Merged

(feat): merging analysis tests#9872
eytan-starkware merged 1 commit intomainfrom
eytan_graphite/_feat_merging_analysis_tests

Conversation

@eytan-starkware
Copy link
Copy Markdown
Contributor

@eytan-starkware eytan-starkware commented Apr 23, 2026

Summary

Consolidates the separate def_site_test.rs and dom_test.rs test modules into a single unified test_analysis runner in test.rs. The new runner dispatches to the appropriate analysis (dom or def_site) based on an analysis arg declared in each test data file. A Display implementation is added to Dominators so it can render its own output, removing the inline formatting logic that previously lived in the test module. Test data files are updated to use test_analysis(analysis: dom) and test_analysis(analysis: def_site) as their runner names, and the dominators output section is renamed to result for consistency across both analyses.


Type of change

Please check one:

  • Bug fix (fixes incorrect behavior)
  • New feature
  • Performance improvement
  • Documentation change with concrete technical impact
  • Style, wording, formatting, or typo-only change

Why is this change needed?

The two analysis test modules (def_site_test.rs and dom_test.rs) were nearly identical in structure, each duplicating the same boilerplate for setting up the database, lowering the function, and writing results. Adding a new analysis required creating an entirely new test module. The unified runner reduces duplication and makes it trivial to add new analyses with only a new match arm and a test data file.


What was the behavior or documentation before?

Each analysis had its own dedicated test module with its own test_file_test! macro invocation, its own runner function, and its own output key name (dominators for dom, result for def_site).


What is the behavior or documentation after?

A single test_analysis runner handles all file-based analysis tests. The analysis arg in each test data file selects which analysis to run. All analyses use a uniform result output key. Dominators implements Display directly rather than relying on ad-hoc formatting in the test module.


Related issue or discussion (if any)


Additional context

Adding support for a new analysis in the future only requires a new match arm in test_analysis and a corresponding test_data/<name> file — no new test module needed.

@reviewable-StarkWare
Copy link
Copy Markdown

This change is Reviewable

@eytan-starkware eytan-starkware marked this pull request as ready for review April 23, 2026 12:22
@cursor
Copy link
Copy Markdown

cursor Bot commented Apr 23, 2026

PR Summary

Low Risk
Low risk: changes are limited to test infrastructure and Debug formatting for analysis result wrappers, with no impact on lowering/analysis algorithms.

Overview
Consolidates the file-based analysis golden tests into a single test_analysis runner in analysis/test.rs, dispatching by an analysis arg and standardizing outputs under a result key.

Replaces bespoke test-side formatting by adding custom Debug implementations for DefSites and Dominators, and simplifies dominator_test.rs to only unit-test dominates queries. Updates test_data/def_site and test_data/dominator expectations to match the new runner and debug output, and removes the standalone def_site_test.rs module.

Reviewed by Cursor Bugbot for commit 5444b1b. Bugbot is set up for automated code reviews on this repo. Configure here.

Copy link
Copy Markdown
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@orizi made 1 comment.
Reviewable status: 0 of 7 files reviewed, 1 unresolved discussion (waiting on eytan-starkware and TomerStarkware).


crates/cairo-lang-lowering/src/analysis/test.rs line 29 at r1 (raw file):

    "src/analysis/test_data",
    {
        dom: "dom",

inator

@eytan-starkware eytan-starkware changed the base branch from eytan_graphite/_feat_dominator_analysis_using_forward_dataflow to graphite-base/9872 April 30, 2026 13:20
@eytan-starkware eytan-starkware force-pushed the eytan_graphite/_feat_merging_analysis_tests branch from aeb10da to 5d2a519 Compare May 3, 2026 10:59
@eytan-starkware eytan-starkware changed the base branch from graphite-base/9872 to main May 3, 2026 10:59
@eytan-starkware eytan-starkware force-pushed the eytan_graphite/_feat_merging_analysis_tests branch from 5d2a519 to 4685881 Compare May 3, 2026 11:01
Copy link
Copy Markdown
Contributor Author

@eytan-starkware eytan-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eytan-starkware made 1 comment.
Reviewable status: 0 of 11 files reviewed, 1 unresolved discussion (waiting on orizi and TomerStarkware).


crates/cairo-lang-lowering/src/analysis/test.rs line 29 at r1 (raw file):

Previously, orizi wrote…

inator

Done.

Copy link
Copy Markdown
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@orizi reviewed 11 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on eytan-starkware and TomerStarkware).


crates/cairo-lang-lowering/src/analysis/test.rs line 260 at r2 (raw file):

        ConcreteFunctionWithBodyId::from_semantic(db, test_function.concrete_function_id);
    let mut lowered = db.lowered_body(function_id, LoweringStage::PostBaseline).cloned().unwrap();
    let extra = lowered.variables.iter().next().unwrap().1.clone();

doc this is the actual interesting line.

Code quote:

    let extra = lowered.variables.iter().next().unwrap().1.clone();

@eytan-starkware eytan-starkware force-pushed the eytan_graphite/_feat_merging_analysis_tests branch from 4685881 to 58b48e9 Compare May 3, 2026 12:03
Copy link
Copy Markdown
Contributor Author

@eytan-starkware eytan-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eytan-starkware made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on orizi and TomerStarkware).


crates/cairo-lang-lowering/src/analysis/test.rs line 260 at r2 (raw file):

Previously, orizi wrote…

doc this is the actual interesting line.

Done.

@eytan-starkware eytan-starkware force-pushed the eytan_graphite/_feat_merging_analysis_tests branch from 58b48e9 to 5444b1b Compare May 3, 2026 12:10
Copy link
Copy Markdown
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

@orizi reviewed 1 file and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on TomerStarkware).

@eytan-starkware eytan-starkware added this pull request to the merge queue May 3, 2026
Merged via the queue into main with commit d534bf9 May 3, 2026
54 checks passed
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.

3 participants