Got even more tests working

[savetheclocktower]

At this point, everything that's commented out is that way because it's testing stuff that just works differently in this package, or that I don't think is worth adding.

At this point, I'd be OK with putting out a first release if we can prove that it works for actual human beings on Windows, and not just in CI. The log statements should get cleaned up, of course, though I'll probably keep some in as console.debugs that log only when Atom is in dev mode.

Some things in here worth mentioning:

• I’m skipping the test that ensures that a file that’s ignored in package.json will nonetheless be linted if it’s in the root and there’s also an .eslintignore file that doesn’t ignore it. I spent 90 minutes wading through ESLint internals trying and failing to figure out how the Rube Goldberg machine that determines its configuration was doing the expected thing from the command line, but not from the equivalent ESLint#lintText command. Finally, out of desperation, I switched our call from lintText to lintFiles, and suddenly it passed.

  It’s always possible that I’ve just overlooked the world’s simplest typo somewhere in worker.js. But failing that, there’s no earthly reason why ESLint shouldn’t treat those paths exactly the same (provided I pass filePath when I call ESLint#lintText — which I do). Also, if someone is relying on this behavior, I’ll happily tell them to go use VSCode instead.

• Most of what I thought I learned about cwd was wrong. If you set it to a file’s directory, it’ll happily traverse upward and assemble config options from a bunch of .eslintrcs, but it’ll only consume one .eslintignore, and it has to live in cwd. So I replicated the logic from getRelativePath, but with a tweak.

  I think it’s fine to start at the file to be linted and then stop at the first .eslintignore you find, and treat that as cwd. But as far as I can tell, if you open a file in a project that has ESLint but is missing an .eslintignore file altogether, linter-eslint will happily keep traversing upward in search of one — all the way to the volume root if need be — before giving up and using the project path.

  In trying to use the closest .eslintignore to the file, we’re already doing more than the eslint binary does, so there’s no specific behavior that we need to replicate here. I think it’s weird to try to lint a file inside a project only to discover that it won’t get linted because it matches an ignore pattern defined by a file that’s not even in the project. So I made the opinionated decision to stop at the project root.

  I think that this is a good idea. The only people who I think could possibly have relied on the previous behavior are people who work in a monorepo and treat each package as a separate project but also delegate their .eslintignore-ing to a file at the repo root… and I’ll bet there are five people on Earth who fit that description, and four of them probably use WebStorm.

• I changed one test fixture because it was relying on the no-semicolon rule that was present in linter-eslint that I have selfishly changed into a rule that requires semicolons in linter-eslint-node.

• I got rid of the “Lint HTML Files” option and replaced it with extra description text for the “Scopes” option. Also got rid of “Path to local node_modules”; I can add this back if anyone requests it, but I doubt they will. Added a “Use cache” advanced option that bypasses the ESLint instance cache for diagnostic purposes.

• My decision to go with a small post-fix-job message — outcome of the job in the message field, and no description field at all — is complicating the tests, which expect a certain fixed string to be the “subject” of the notification. If the fix job doesn’t fix the expected number of things, the message won’t be what we expect, and the notification promise will timeout instead of failing quickly. I still like the more compact success message, so I might try to have it both ways here by putting something predictable at the front of the message if we’re in spec mode.

[savetheclocktower]

The other thing that occurred to me: we have no tests for the node-resolution logic or for the ability to specify per-project options via .linter-eslint. I'll start writing tests for the latter tonight, but the former feels hard.

If someone wants to contribute to this package in the future, I don't think we should make them download various specific versions of node into known locations on their drive just so we can run some specs. This feels like a CI task. Is it possible to define specs that only run on CI?

[savetheclocktower]

I knew those new config specs were too good to be true; they felt too easy to write. I have no idea why the “if we rename .linter-eslint” test is failing in the way that it is; that promise should reject after five seconds, not 60.

[UziTech]

The test runner is setup to give a ci a little bit more time. The default timeout for ci is 60 seconds.

[UziTech]

We can use environment variables to only run tests in certain environments.

[savetheclocktower]

OK, it turns out to be a problem with async tests in general. I can't get any sort of async strategy to work, including either of these simple test cases:

'use babel';

function wait (ms) {
  return new Promise((resolve) => {
    setTimeout(resolve, ms);
  });
}

describe('Test file', () => {
  it('does something with setTimeout', (done) => {
    setTimeout(() => {
      expect(typeof 1).toBe('number');
      done();
    }, 100);
  });

  it('does something with async/await', async () => {
    await wait(1000);
    expect(typeof 1).toBe('number');
  });
});

They both timeout. Experiencing this on both Windows and macOS, so there's something happening in the Jasmine internals that I don't understand, or perhaps in the transpilation. This despite the fact that all the existing tests that use await are working.

I wonder if it's got to do with the clock being mocked? If I do jasmine.clock().install() in a test, it errors with Error: Jasmine Clock was unable to install over custom global timer functions. Is the clock already installed?

[savetheclocktower]

OK, I was ready to give up on this several times, but I've got this test passing locally now on Windows, at least via GUI.

1. The core issue on Windows is that when we set the project path, Atom starts an async job to set up a watcher on that path. We need to wait for that watcher to be ready before proceeding, or else it won't realize that we've renamed the file.
2. Jasmine does indeed wrap setTimeout with something; preserving the unwrapped version and exporting it from the helpers file seems to do the trick. I was confused about 5-seconds-versus-60 because my untilConfigChanges function explicitly waits for 5000 ms before giving up, regardless of environment, but that wasn't doing anything because it was calling the wrapped version of setTimeout. Should work just fine now.

And as I write this I notice that CI is still failing on Windows, but at least it's passing on Linux. My woes continue.

[savetheclocktower]

That one failing test on Windows passes for me locally, so I don't know what's going on. It seems weird for such a simple test to be flaky.

[UziTech]

It doesn't seem to be just windows. It also seems like different tests are failing. I'm thinking we need to make the tests more granular so we aren't relying on atom as much.

[savetheclocktower]

The tests that claim to be failing are direct equivalents of tests in linter-eslint. I suspect the timeout errors are triggered by config-spec.js tests and just not getting matched up correctly, because I believe all those tests were passing before I started writing config tests.

But now that I've skipped the one test that was giving me trouble, it's still having sporadic timeout issues, so I'll try a couple more things. Having to diagnose test failures in CI that I can't reproduce on my machine might be my least favorite task in all of computer science.

[savetheclocktower]

OK, my nightmare is approaching an end. And yours, too, if you've been getting all these CI failure emails as well.

The new failures in linter-eslint-node-spec.js were caused by a Config.onConfigDidChange handler firing too early and making us think that nodeBin had changed when it hadn't. That caused the worker script to get killed and re-initialized with the “new” version of Node, which somehow was consistently happening inside of one particular test on Windows CI, and another particular test on Ubuntu CI. That caused a linting job to get orphaned and timeout.

Knowing now that those failures weren’t just weird side effects of config-spec.js specs, I took another look at those. Any failure that happened consistently in Windows CI is something that I could reproduce about one time out of ten locally. So, after a bunch of trial and error, I managed to isolate the flakiness to a race condition just after file modifications. await editor.save() triggers a file change modification, and those are not guaranteed to be synchronous. So whenever we save a file, or rename it, or something, we’ve got to cede control to be sure that those callbacks have run before we try to assert stuff.

In my experiments on my Windows machine, even await wait(0) was enough to get these tests passing consistently except maybe one time in a hundred. I made them all await wait(1000) just for safety. That got everything passing on all platforms.

I hate waiting for fixed amounts of time, and I’d be glad to find a different way to know when it’s safe for those tests to proceed, but the most obvious way to do that would be to use the same APIs that are part of what I’m testing in the first place. That feels icky, but if I have to revisit these tests, I will live with that icky feeling.

And, yes, it’s always an option to test Config in isolation. But the integration points between the main module and Config are important, too, because they’re the parts that save you from having to reload your project window whenever you make a change to your config. If these tests prove to be flaky no matter what I do, I'll rewrite them to call Config.rescan() and Config.update() manually.