Skip to content

TOOLS-4148 Rewrite all the bsondump tests in Go and delete the JS tests for this#923

Open
autarch wants to merge 1 commit into03-25-add_a_new_test_in_common_for_our_handling_of_sigpipefrom
03-19-rewrite_all_the_bsondump_tests_in_go_and_delete_the_js_tests_for_this
Open

TOOLS-4148 Rewrite all the bsondump tests in Go and delete the JS tests for this#923
autarch wants to merge 1 commit into03-25-add_a_new_test_in_common_for_our_handling_of_sigpipefrom
03-19-rewrite_all_the_bsondump_tests_in_go_and_delete_the_js_tests_for_this

Conversation

@autarch
Copy link
Copy Markdown
Collaborator

@autarch autarch commented Mar 19, 2026

A few of the JS tests were not ported, per the plan in docs/superpowers

Copy link
Copy Markdown
Collaborator Author

autarch commented Mar 19, 2026

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

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

@autarch autarch force-pushed the 03-19-rewrite_all_the_bsondump_tests_in_go_and_delete_the_js_tests_for_this branch from 629d309 to 314536f Compare March 20, 2026 16:53
@autarch autarch force-pushed the 03-18-migration_annotate_js_tests_with_verified_go_coverage_triage branch from 2d4c1a3 to 8602a4d Compare March 20, 2026 16:53
@autarch autarch force-pushed the 03-19-rewrite_all_the_bsondump_tests_in_go_and_delete_the_js_tests_for_this branch from 314536f to af5f557 Compare March 20, 2026 18:31
@autarch autarch force-pushed the 03-18-migration_annotate_js_tests_with_verified_go_coverage_triage branch 2 times, most recently from 951bc97 to 7a3fa74 Compare March 20, 2026 20:34
@autarch autarch force-pushed the 03-19-rewrite_all_the_bsondump_tests_in_go_and_delete_the_js_tests_for_this branch 2 times, most recently from 6cbeff6 to 5c182e0 Compare March 20, 2026 20:35
@autarch autarch force-pushed the 03-18-migration_annotate_js_tests_with_verified_go_coverage_triage branch from 7a3fa74 to be4d1de Compare March 20, 2026 20:35
@autarch autarch force-pushed the 03-19-rewrite_all_the_bsondump_tests_in_go_and_delete_the_js_tests_for_this branch from 5c182e0 to 6a8bc27 Compare March 20, 2026 20:38
@autarch autarch force-pushed the 03-19-rewrite_all_the_bsondump_tests_in_go_and_delete_the_js_tests_for_this branch from 6a8bc27 to ce94d63 Compare March 20, 2026 21:26
@autarch autarch force-pushed the 03-18-migration_annotate_js_tests_with_verified_go_coverage_triage branch from be4d1de to 13b1165 Compare March 20, 2026 21:26
@autarch autarch marked this pull request as ready for review March 20, 2026 21:27
@autarch autarch requested a review from a team as a code owner March 20, 2026 21:27
@autarch autarch requested review from FGasper and mankawal and removed request for a team and mankawal March 20, 2026 21:27
Copy link
Copy Markdown
Collaborator

@FGasper FGasper left a comment

Choose a reason for hiding this comment

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

Looks like the refactor reduced coverage in one test and imprudently skipped another.

Comment on lines +357 to +363
badFiles := []string{
"testdata/bad_cstring.bson",
"testdata/bad_type.bson",
"testdata/invalid_field_name.bson",
"testdata/partial_file.bson",
"testdata/random_bytes.bson",
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The refactoring of these tests makes it kind of hard to compare with the originals.

It might be nice if the AI privileged line-for-line translation rather than trying to refactor.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

It can do that, but do we really want that? If a human did this work I'm sure they'd do this same refactor. The way the JS test was written is just wrong. It should have used a loop!

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I agree that the original test’s copy-paste-ness was excessive.

The refactor, though, makes it hard to ensure that the test was ported faithfully. I found one spot in this PR where Claude dropped something earlier; verifying that there isn’t something else is easier when the Go code resembles the JS.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I’m not blocking on this, btw … but it might be a good parameter to give the AI in future translations.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

If you think the existing set of PRs are a lot to review, imagine effectively doubling the number of PRs by breaking it up into "rewrite the JS test exactly into Go" and then "rewrite that into sane Go code". Is that a tradeoff we really want? I think the real problem with this PR is that I should've just done one test at a time.

@autarch autarch force-pushed the 03-19-rewrite_all_the_bsondump_tests_in_go_and_delete_the_js_tests_for_this branch from ce94d63 to 93cafd5 Compare March 23, 2026 15:07
@autarch autarch force-pushed the 03-18-migration_annotate_js_tests_with_verified_go_coverage_triage branch from 13b1165 to aae3a48 Compare March 23, 2026 15:07
@autarch autarch requested a review from FGasper March 23, 2026 15:07
@autarch autarch force-pushed the 03-18-migration_annotate_js_tests_with_verified_go_coverage_triage branch from aae3a48 to 66aa6b5 Compare March 23, 2026 15:30
@autarch autarch force-pushed the 03-19-rewrite_all_the_bsondump_tests_in_go_and_delete_the_js_tests_for_this branch from 93cafd5 to d6504ac Compare March 23, 2026 15:30
@autarch autarch force-pushed the graphite-base/923 branch from d93b762 to a7a8c12 Compare March 26, 2026 16:53
@autarch autarch changed the base branch from graphite-base/923 to 03-25-add_a_new_test_in_common_for_our_handling_of_sigpipe March 26, 2026 16:54
@autarch autarch changed the base branch from 03-25-add_a_new_test_in_common_for_our_handling_of_sigpipe to graphite-base/923 March 26, 2026 21:36
@autarch autarch force-pushed the 03-19-rewrite_all_the_bsondump_tests_in_go_and_delete_the_js_tests_for_this branch from 94cbc99 to a0ef32f Compare March 26, 2026 21:53
@autarch autarch force-pushed the graphite-base/923 branch from a7a8c12 to 8887a8d Compare March 26, 2026 21:53
@autarch autarch changed the base branch from graphite-base/923 to 03-25-add_a_new_test_in_common_for_our_handling_of_sigpipe March 26, 2026 21:53
@autarch autarch changed the base branch from 03-25-add_a_new_test_in_common_for_our_handling_of_sigpipe to graphite-base/923 March 27, 2026 18:27
@autarch autarch force-pushed the graphite-base/923 branch from 8887a8d to f183501 Compare March 27, 2026 18:27
@autarch autarch force-pushed the 03-19-rewrite_all_the_bsondump_tests_in_go_and_delete_the_js_tests_for_this branch from a0ef32f to b457216 Compare March 27, 2026 18:27
@autarch autarch changed the base branch from graphite-base/923 to 03-25-add_a_new_test_in_common_for_our_handling_of_sigpipe March 27, 2026 18:27
@autarch autarch changed the base branch from 03-25-add_a_new_test_in_common_for_our_handling_of_sigpipe to graphite-base/923 March 27, 2026 18:43
@autarch autarch force-pushed the 03-19-rewrite_all_the_bsondump_tests_in_go_and_delete_the_js_tests_for_this branch from b457216 to 9db3a63 Compare March 27, 2026 20:27
@autarch autarch force-pushed the graphite-base/923 branch from f183501 to 4e2c586 Compare March 27, 2026 20:27
@autarch autarch changed the base branch from graphite-base/923 to 03-25-add_a_new_test_in_common_for_our_handling_of_sigpipe March 27, 2026 20:27
@autarch autarch force-pushed the 03-19-rewrite_all_the_bsondump_tests_in_go_and_delete_the_js_tests_for_this branch from 9db3a63 to de8522d Compare March 27, 2026 21:12
@autarch autarch force-pushed the 03-25-add_a_new_test_in_common_for_our_handling_of_sigpipe branch from 4e2c586 to 1563a49 Compare March 27, 2026 21:12
@autarch autarch changed the base branch from 03-25-add_a_new_test_in_common_for_our_handling_of_sigpipe to graphite-base/923 March 27, 2026 21:13
@autarch autarch force-pushed the 03-19-rewrite_all_the_bsondump_tests_in_go_and_delete_the_js_tests_for_this branch from de8522d to 3f6ec50 Compare March 27, 2026 21:14
@autarch autarch force-pushed the graphite-base/923 branch from 1563a49 to f440194 Compare March 27, 2026 21:14
@autarch autarch changed the base branch from graphite-base/923 to 03-25-add_a_new_test_in_common_for_our_handling_of_sigpipe March 27, 2026 21:14
@autarch autarch force-pushed the 03-25-add_a_new_test_in_common_for_our_handling_of_sigpipe branch from f440194 to beca1d3 Compare March 27, 2026 21:15
A few of the JS tests were not ported, per the plan in docs/superpowers
@autarch autarch force-pushed the 03-25-add_a_new_test_in_common_for_our_handling_of_sigpipe branch from beca1d3 to 3f1896d Compare March 27, 2026 21:15
@autarch autarch force-pushed the 03-19-rewrite_all_the_bsondump_tests_in_go_and_delete_the_js_tests_for_this branch from 3f6ec50 to 4243504 Compare March 27, 2026 21:16
@autarch autarch changed the base branch from 03-25-add_a_new_test_in_common_for_our_handling_of_sigpipe to graphite-base/923 March 27, 2026 21:16
@graphite-app graphite-app bot changed the base branch from graphite-base/923 to 03-25-add_a_new_test_in_common_for_our_handling_of_sigpipe March 27, 2026 21:16
@autarch autarch force-pushed the 03-19-rewrite_all_the_bsondump_tests_in_go_and_delete_the_js_tests_for_this branch from 4243504 to 41037a4 Compare March 27, 2026 21:16
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