Skip to content

Conversation

@Emyrk
Copy link
Member

@Emyrk Emyrk commented Nov 19, 2025

Refactoring ConvertState is something we should eventually do. This PR adds a golden file unit test for the output of ConvertState (even errors).

That way if a refactor occurs, we can verify the output is unchanged for our test cases.

Copy link
Member Author

Emyrk commented Nov 19, 2025

This stack of pull requests is managed by Graphite. Learn more about stacking.

golden file test is to prevent regressions in behavior. It is going
to be used as a benchmark for a refactor
@Emyrk Emyrk force-pushed the stevenmasley/convert_state_refactor branch from d879360 to 18fa1ff Compare November 19, 2025 17:49
@Emyrk Emyrk force-pushed the stevenmasley/convert_state_refactor branch from 6849ee9 to 7cbbc20 Compare November 19, 2025 18:11
@Emyrk Emyrk force-pushed the stevenmasley/convert_state_refactor branch from 57ea109 to 2ee6b40 Compare November 20, 2025 18:30
@Emyrk Emyrk marked this pull request as ready for review November 20, 2025 18:30
// If the directory is missing these files, we cannot run ConvertState
// on it. So it's skipped.
if srcIdc == -1 || dotIdx == -1 {
continue
Copy link
Contributor

Choose a reason for hiding this comment

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

This should log indicating so.

Copy link
Contributor

@dannykopping dannykopping left a comment

Choose a reason for hiding this comment

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

LGTM, with a couple small nits
I don't need to review again

Thanks a lot for adding these!

@Emyrk Emyrk merged commit a61b8bc into main Nov 21, 2025
31 checks passed
@Emyrk Emyrk deleted the stevenmasley/convert_state_refactor branch November 21, 2025 13:27
@github-actions github-actions bot locked and limited conversation to collaborators Nov 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants