jrochkind
commented
2 years ago Your note "This is a work-around until the support for Webpacker is resolved" surprises me, becuase I thought the tests not testing with Webpacker at all, but only with sprockets.
- have you confirmed that the tests are definitely testing with Webpacker? And that that is the source of the problem? I am curious how you know it's the source of the problem.
- I actually don't understand how it could be including the browse_everything javascript at all with webpacker, like how any tests using that JS could be passing with webpacker not just this one test. I thought this gem hadn't yet solved the problem of including Js via webpacker at all. Do you have any insight there? If there is a way to do that, it would be great to include in README!
- Is it intentional that the tests are testing with webpacker? If they were meant to only be testing with sprockets, then would a fix be making the tests only run with sprockets and not webpacker again?
- Perhaps the comment should say "This test is disabled until the support for Webpacker is resolved", rather than "this is a work-around" -- the "work-around" of course is disabling the test! But if there's a way to link to any more information about what the relevant problem with "support for Webpacker" is, that would be helpful to link to! So someone coming along later has some sense of the nature of the problem that resulted in disabling this test. It seems not great to disable this test of fairly important functionality -- perhaps we should at least create a Github Issue to re-enable the test, so it's at least hypothetically on the radar as work to do? That would be a good place to document more about what is known about the problem and how it's related to webpacker, and that comment in source could then be linked to that issue?
jrgriffiniii
Rebases the changes proposed with https://github.com/samvera/browse-everything/pull/377
Additionally, this also temporarily disables the Turbolinks test suite blocking the merging of pull requests.