Manually Decaf tabs package Specs

[confused-Techie]

In this PR I've removed the last CoffeeScript remaining in the tabs package. Which were the specs.

The main portion of this PR is rather simple. Removing useless returns and switching to a more modern style by removing inline anonymous function declarations and instead using arrow functions.

Otherwise the most complex part relates to the event-helper files, which contain the details below:

The spec files here originally had event-helper.js and event-helper.coffee so it was difficult to tell which was more up to date.

And seems they were both still in use. With the following line being at the top of tabs-spec.coffee

let {triggerMouseEvent, ..., buildDragEnterLeaveEvents} = require("./event-helpers.coffee");
({buildDragEnterLeaveEvents} = require("./event-helpers"));

Then this wasn't as simple as decafing event-helpers.coffee since it seems the buildDragEnterLeaveEvents only existing in the JS, and relied on an internal function to both specs files that had different implementations between each.

So I've used machine decaf on event-helpers.coffee and added the function buildDragEnterLeaveEvents into the decaffed code, copied directly from the existing JS.

From there buildMouseEvent was the function that existed in both specs but differred slightly between the two. So this had to be rewritten to support both the usage from event-helpers.coffee and event-helpers.js but otherwise looks fine, with all tests passing locally.

---

Again if this PR is accepted then it'll be merged directly to main, rather the the machine decaf branch that it currently targets for ease of review.

[confused-Techie]

Alright @pulsar-edit/core this PR is good to review, all tests are passing without issue, matching the amount of tests we would expect.

It may be worth noting that we do have on unexpected failure of tests the one-dark-ui test is failing, but this is because of the RipGrep download and is a known issue.

[confused-Techie]

Alright quick update. Further reviewed the tests, since there were some unexpected failures, but seemed to be related to like mentioned above the RipGrep download. But that isn't totally the case. Going off of our current expected failure rate here's what we have for this PR's tests currently:

• one-dark-ui: Errors out downloading RipGrep
• background-tips: Errors out downloading RipGrep
• find-and-replace: 57 Failures (51 Expected Failures)
• symbols-view: 4 Failures (2 Expected Failures)
• tree-view: 2 Failures (2 Expected Failures)
• update-package-dependencies: Errors our downloading RipGrep

So while outside the scope of this original PR I'll take a look at ensuring we have a proper amount of passing tests.
Firstly by obviously applying the known fix for RipGrep downloads, and otherwise will try to investigate why we have 8 tests failing in unrelated packages of symbols-view and find-and-replace for this PR.

[confused-Techie]

Alright, with one small change to the test runner we now have expected failure rates.

• symbols-view: 2 Failures (2 Expected)
• tree-view: 2 Failures (2 Expected)
• find-and-replace: 43 Failures (51 Expected)

It's worth noting for find-and-replace that without any changes to relevant code we are seeing some variation to the amount of tests passed. So this may need to be chalked up to a flaky test and I've updated the status of this test on Discord to reflect this. Otherwise these tests are looking good, as for the 2 mystery failures on symbols-view this may have been related to some sort of https GitHub download during build time or may also be a flaky test. Either way this PR is now good to fully review.

[savetheclocktower]

Oof, I spent 20 minutes decaffeinating tabs-spec.coffee as part of fixing #349 before I realized that this PR existed :)

I'll wait until this lands on master before opening my PR.

[confused-Techie]

Oof, I spent 20 minutes decaffeinating tabs-spec.coffee as part of fixing #349 before I realized that this PR existed :)

I'll wait until this lands on master before opening my PR.

Sorry to hear we doubled up on work, but glad to see a fix is in for that! Super awesome, and good point to mention, anyone from @pulsar-edit/core wanna take a look? Or considering the scope and the fact that all tests are passing should we be good to merge?

[DeeDeeG]

I can confirm the tests still pass with this, both for me locally and I observe it passes in CI also.

Shouldn't make a difference, but just in case: I also loaded the package from this branch into my editor, using ppm install and ppm link in the packages/tabs dir, and reloaded my editor. Tabs still work as expected. (Which makes sense... this PR or its base branch don't modify the source code after all...)

---

More thoughts:

I have some minor things I notice with usage of undefined vs null that I wonder about, however this stuff is all internally used in the packages/tabs tests only, so certain details don't matter, this does not run in production, and we practically speaking will never receive unexpected inputs. (All inputs are stably defined in the test files, to the extent changes in the rest of the editor don't invalidate the current test logic, I suppose.)

I implicitly trust these efforts, and can apply somewhat less scrutiny by default since it is passing. (Although at some risk of "false pass" test cases if the logic turns out to have been incorrectly translated to clean JS?) If it makes sense to, I can apply more scrutiny than I have, and do more rounds of review feedback, but I feel from a quick look that it is okay.

(These decaf PRs are going to take a very long time to earnestly and fully review, I think? So I am honestly not looking over every line in fine detail. Let me know if you want my thoughts on the defined vs null stuff, but I am aware this would take lots more time for back-and-forth, and may not be worth it. It's a real trade-off, IMO, so I would go either way with it.)

UPDATE: I think I spotted a tiny (one character) genuine mistake (albeit one with no apparent impact, since I think this function's inputs are defined in this file and I don't think they're ever undefined or null). I proposed a fix in a suggested diff below.

[DeeDeeG]

PR was technically fine before my round of review, now review comments are addressed, double-approved, two thumbs up, 👍 👍 shipit ™️ 🚀

Edit to add: FWIW I tried my best to read through all the 634 +/- changes to tabs-spec.js and nothing jumped out at me as needing comment. Just removing presumably unneeded return statements, converting function () {} to arrow functions () =>. Which looks all good to me. Nice!

Nothing left to look at, so the whole thing LGTM.

[confused-Techie]

@DeeDeeG Appreciate the review! I'll go ahead and merge this one

[savetheclocktower]

@confused-Techie: apologies for making this about my fix for #349, but what's the timetable for merging machine-decaf-tabs-spec into master? Would you prefer that I wait until that happens, or else open my PR against machine-decaf-tabs-spec for review purposes?

[confused-Techie]

@confused-Techie: apologies for making this about my fix for #349, but what's the timetable for merging machine-decaf-tabs-spec into master? Would you prefer that I wait until that happens, or else open my PR against machine-decaf-tabs-spec for review purposes?

No worries, the proper branch has already been merged into master in #367

I intended to change the target branch here after review, but had forgotten todo so