[find-and-replace] Fix some bugs in the spec suite

[savetheclocktower]

When find-and-replace had a flaky failure in CI earlier today, I looked at its output and found it… perplexing.

I saw a lot of this output:

FindView > when find-and-replace:toggle is triggered > it toggles the visibility of the FindView ERROR: Dev tools attempted to open
[pass]
FindView > when the find-view is focused and window:focus-next-pane is triggered > it attaches FindView to the root view ERROR: Dev tools attempted to open
[pass]
FindView > find-and-replace:show-replace > it focuses the replace editor ERROR: Dev tools attempted to open
[pass]
FindView > find-and-replace:show-replace > it places the current selection in the replace editor ERROR: Dev tools attempted to open
[pass]

It unnerved me that the opening of dev tools claimed to be triggered on every test, since that typically suggests an error. And the fact that all the tests were passing anyway told me that an error was being suppressed incorrectly.

Even worse: each test was taking several seconds to pass. The suite crawled along at a glacial pace. I figured something weird was going on.

In truth, there were two root causes, neither of which was quite as alarming as it seemed:

• The Dev tools attempted to open message was a false positive. We were incorrectly mocking atom.openDevTools (the function which opens the dev tools imperatively) and immediately logging this message instead of the intended behavior of logging the message when atom.openDevTools() is called.

  (The purpose of mocking this function in order to suppress the opening of developer tools is simply that it prevents the test suite from losing focus, since that would cause further failures on specs that test the focus behavior. The errors still get logged to the console, so no actual failures are being suppressed.)

• The slowness of the suite was the result of genPromiseToCheck (the equivalent of what we call waitsFor in the pre-async tests) and some hastily written predicates with bugs in them. genPromiseToCheck (which I’ve renamed waitForCondition here for clarity and consistency) waited up to 4000ms for a given condition to be met; but it resolved if it hit that timeout limit. Hence the test suite would proceed after four seconds.

  If the predicate was correctly written, this didn’t have any real effect — the next assertion would fail. If the predicate was incorrectly written in a way that caused test failures, then that would already have been noticed and fixed. But some of the predicates were incorrectly written in such a way that did not cause test failures — just slowed them down by waiting for something that was never going to happen. In these cases, await genPromiseToCheck() was acting as the equivalent of await wait(4000); it explains why all these tests passed anyway, but also why the suite was so slow.

  I rewrote waitForCondition so that it rejects after four seconds; if the condition is never met, the spec is automatically failed. This matches the behavior of waitsFor. And fixing it caused a number of other test failures, all of which were by definition failures in the logic of waitForCondition predicate functions.

  One of the faulty predicates was called from resultsPromise() in the project view specs file. This was regrettable because await resultsPromise() was present in practically every spec. I replaced it with a function that I could reason about; in a couple cases where that function’s logic wasn’t sufficient, I threw in an await wait(500) or the like just so I could move on without devoting a whole lot of time to this.

  Other failures were mainly the results of typos in predicates, like typing querySelectorAll instead of querySelector. They were easily fixed.

• In some places I noticed errors in the console that seemed not to affect the tests at all, probably because they were side effects that happened during async operations. I think I found the root cause of all of them:

  On some views, we destroy the model when the destroy method is called, but other code can still run that assumes this.model is present. In some cases, this bug is the result of incomplete disposal (retaining subscriptions that should not be retained), so the fix was straightforward. In other cases, the cause was not clear, so I put guards around them and moved on.

Testing

The find-and-replace package test suite should pass in CI and should pass a lot faster than before. You also shouldn’t see any ERROR: Dev tools attempted to open messages unless and until actual errors occur.