Skip to content

Remove upstream tests that also exist in flambda-backend tests #3941

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jvanburen
Copy link
Contributor

No description provided.

@jvanburen jvanburen requested a review from gretay-js April 29, 2025 15:51
@jvanburen jvanburen force-pushed the jvb.remove-redundant-upstream-tests branch from 5fbd0ff to 6ec0ac2 Compare April 29, 2025 19:33
@goldfirere
Copy link
Collaborator

goldfirere commented Apr 29, 2025

Having two copies of the tests is definitely bad. But is there a good reason to favor the flambda-backend testsuite over the upstream one? I expect we'll have an easier time upstreaming flambda2 if we don't have to overhaul the testsuite at the same time. I don't really know what I'm talking about here, and maybe you all have conferred (in which case just tell me to wander off), but I saw this go by and was curious.

@jvanburen jvanburen removed the request for review from gretay-js April 30, 2025 13:05
@jvanburen
Copy link
Contributor Author

The reason is that the current test suite already overwrites the upstream tests with the flambda2 ones! Having a separate unused copy of the tests is super confusing IMO, and already wasted at least an hour of my time

@Gbury
Copy link
Contributor

Gbury commented Apr 30, 2025

I'm with @goldfirere on this one: as much as possible, we should use the existing upstream testsuite harness for tests (because upstreaming). Only for specific tests that cannot be made compatible should we keep separate tests (and even then, I suspect it would be possible to adapt these to ocamltest, since it allows to use arbitrary scripts).

@jvanburen
Copy link
Contributor Author

someone else will have to confirm but i believe this does use the upstream test harness, the tests are copied into the upstream directory by the makefile before running!

@bclement-ocp
Copy link
Contributor

Something slightly fishy seems to be going on with these tests because I think we are not just using new versions of existing tests but also dropping a bunch of them. The files that are there (I just looked at poll_attr_both and poll_attr_user) also seem to be identical in both directories. Might be an artifact of the pivot root @mshinwell @lukemaurer?

$ ls flambda-backend/testsuite/tests/asmcomp/
poll_attr_both.compilers.reference  poll_attr_inserted.compilers.reference  poll_attr_prologue.compilers.reference  poll_attr_user.compilers.reference
poll_attr_both.ml                   poll_attr_inserted.ml                   poll_attr_prologue.ml                   poll_attr_user.ml

$ ls testsuite/tests/asmcomp/
0001-test.compilers.reference      evaluation_order.reference   is_static.ml                            poll_attr_prologue.ml               regression_value_kinds.ml      static_float_array_flambda_opaque.ml
0001-test.ml                       func_sections.arm.reference  lift_mutable_let_flambda.ml             poll_attr_user.compilers.reference  run.ml                         try_checkbound.ml
bind_tuples.ml                     func_sections.ml             optargs.ml                              poll_attr_user.ml                   select_addr.ml                 unrolling_flambda2.ml
compare.ml                         func_sections.reference      poll_attr_both.compilers.reference      polling.c                           select_addr.reference          unrolling_flambda.ml
compare.reference                  func_sections.run            poll_attr_both.ml                       polling_insertion.ml                simple_float_const.ml
evaluation_order_broken.ml         is_in_static_data.c          poll_attr_inserted.compilers.reference  prevent_fma.ml                      simple_float_const_opaque.ml
evaluation_order_broken.reference  is_static_flambda_dep.ml     poll_attr_inserted.ml                   register_typing.ml                  staticalloc.ml
evaluation_order.ml                is_static_flambda.ml         poll_attr_prologue.compilers.reference  register_typing_switch.ml           static_float_array_flambda.ml

@Gbury
Copy link
Contributor

Gbury commented Apr 30, 2025

someone else will have to confirm but i believe this does use the upstream test harness, the tests are copied into the upstream directory by the makefile before running!

Right, but in that case, why not have/commit the files at their correct place from the start ? Having separate copies that we have to copy and replace the original with seem like an really unnecessary complication (compared to just adding/changing the files in the testsuites/tests/** directory).

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants