Remove Benchmark Startup Mode

[confused-Techie]

This PR removes the ability to startup in Benchmark mode.

This change was discussed and decided to move forward on Discord, as this allows us to remove some dependencies, as well as cut down on our codebase.

Which cutting down on our codebase and simplifying our code seems integral to survive with our small development team.

---

In further detail, there are two methods to start the editor in benchmark mode, one of which seems likely to come from a CLI flag when starting the editor, and the other is listening on the IPC bus. If either of these methods are used the editor will log a message for the user to review stating that benchmark mode is no longer supported and has been removed from the editor. This can help prevent integrations erroring out, and users unexpectedly seeing the editor fail to launch. Hopefully this can help alleviate any confusion this change could cause. And as such, any startup options to launch with benchmarks have been left in, so that users can reach these messages. At a later time the startup options for launching in benchmark mode can be fully removed, but it may be a good idea to allow a grace period of leaving this in.

[DeeDeeG]

Overall looks good!

There are a couple more files that reference the benchmark mode, most of these, I think:

$ git grep --name-only benchmark
benchmarks/benchmark-runner.js
menus/darwin.cson
menus/linux.cson
menus/win32.cson
pulsar.sh
resources/win/pulsar.cmd
script/lib/copy-assets.js
script/test.js
spec/main-process/atom-application.test.js
src/initialize-benchmark-window.js
src/main-process/atom-application.js
src/main-process/main.js
src/main-process/parse-command-line.js
src/main-process/start.js
src/register-default-commands.js
src/workspace-element.js

• menus/*.cson define a user-facing menu item to "Run Benchmarks", so that should be removed.
• A few of the repo's tests expect to interact with --benchmark or ``--bnchmark-test` mode, so ideally those references should be removed/cleaned up as well.
• script/lib/copy-assets.js tries to copy the benchmarks dir, which might error out if the dir is gone? So the benchmarks/ path should be removed from that file.
• Other miscellaneous stuff in the above-listed files which I haven't had a deep look at yet

I can probably take care a of a couple of these if you want help sorting them out? Not sure if that'll be today or how much of it I can get done quickly, though.

[DeeDeeG]

Okay, I made all the changes I was hoping to make, other than I haven't finished looking into one related spec. (spec/main-process/atom-application.test.js --> search in file for "benchmark". As far as I know, we aren't even running those specs in this repo, so it shouldn't be too much of an issue.)

I pushed to another branch here, you can feel free to take a look and add the commits to your branch (this PR's branch) if you want them. Or if you prefer I do the git-fu to move the commits, I can do that no problem.

The comparison can be viewed here: remove-benchmarks...deedeeg/remove-benchmarks.

(Tech note/GitHub stuff: If a dedicated place is wanted for discussion of my changes, the comparison view gives a button for creating a new Pull Request of my branch targeting this one. If merged, such a PR would add my commits to this PR, all without leaving the GitHub web interface.)

(Note for posterity: As I write this, the branch I posted, and which I am referring to in this comment, is currently at commit 0b45eeb. It is six commits ahead, appended directly off of @confused-Techie's commit 3ec4f4f, which is the current tip of this PR's branch.)

[confused-Techie]

@DeeDeeG I've added your commits, which thanks for that! I appreciate the help. Otherwise as for the tests in atom-application.test.js I attempting trying to go through and test the removal of the tests interacting with Benchmark Mode. But I couldn't get those tests to successfully run at all, even with zero changes. So I think you are right that we don't rely or use those at all, and may be hard to see if we are removing them successfully/properly.

So what do we think of merging as is? Or would you still like to see these removed, even if it could make the tests fail, that is if they could ever run successfully

[DeeDeeG]

I'm working on getting a build of this locally I can run the tests with.

I just want to see what windows the test sees in its "array of open windows/their content" when I delete the line of the test scenario that launches the --benchmark mode window. Then I can update the spec to expect the new, hard-coded correct answer for the scenario and the test should be passing.

If I can't get that far soon, like within 24 hours or so, then I'll let it go for now and try to fix it eventually ™️.

[confused-Techie]

@DeeDeeG that sounds great to me. But yeah when I try to run those tests locally they completely crash. So not sure if they are just broken or if it's windows things. So I hope you have some luck when you get the chance to look at it

[DeeDeeG]

Other than one minor comment where I wasn't sure whether to totally delete an event listener or have it print a message, this looks good to me.

I'm also not 100% clear how this behaves when you pass --benchmark flag when launching... does it launch a new window at all, or just print a message on the CLI? Was not able to test this due to difficulty getting a build going.

I trust if you think it looks good to merge now, so approved. (I will try to do the spec thing soon as a separate PR, if I can get a build made.)

[DeeDeeG]

This event listener now listens for an event which I'm pretty sure will never be triggered. (Because this PR removes everything that used to trigger it. At least it should/I believe it does.)

If so, we can probably delete it entirely, instead of reduce it to only printing a message and then doing nothing.

(This isn't part of the public API stuff, right? Only internal use? Also, I can't see the use-case where an add-on would need to cause a benchmark run to happen... if so, I guess we should keep this so the message will be printed.)

[confused-Techie]

Personally I'd hope to keep it in, just in case we have missed a test that uses it, or for some odd reason some other package or 3rd party service tries to use it. Just so in the event that happens the reason gets logged somewhere. But yeah as far as I know this is an internal integration and not part of the public API. But I do know some other GitHub tools, like GitHub Desktop, integrates pretty closely with Atom. So much more of a just in case thing that we can remove down the line if no issues are discovered.

But yeah being honest I can't see it either. But if it does happen, at least leaving here will avoid the editor crashing or not responding at all.

[confused-Techie]

So being honest @DeeDeeG looking at this, I tried launching with this flag to see what the editor saw, and while using it the editor always still launched just fine, I couldn't actually get to our depreciation message. Using yarn start --benchmark, yarn start -- --benchmark, yarn start --test --benchmark yarn start --test spec --benchmark all launched the editor as if I didn't want any of those things. Or started the test suite as expected.

Additionally tried running the binaries with the --benchmark flag and still just launched them as expected.

So not sure if it's a change in our built tools, or when we stopped using the build script, but can't seem to get it launching attempting to use benchmarks.

So in that case at least it doesn't crash it, or I'm not using them right, but looks like we can merge, Since the only tests that fail are what's expected at this point, and it looks good to both of us.

Thanks for the help here, and maybe once the time comes you can get that last test working properly, we could remove the IPC helper as well, since hopefully that's enough time to confirm things are functioning properly there.