ES6 style guide updates

[w33ble]

Now that we're running all of our code through Babel, and we're starting to use ES6, we need to make some decisions about how and when to use it, and update the style guide accordingly.

Things to consider:

• When we can use fat arrow functions
• When using arrow functions, when to use parens and curly brackets
• let and const
• context in tests (as an alias for describe)
• When and how to unwrap AMD modules
• snake_case vs camelCase file names

[rashidkpc]

As we're making these changes, they really need to be done as separate pull requests.

• Functional changes such as enhancements and bug fixes get proper attention
• Simple style updates can be skimmed

[epixa]

Regarding context and describe, I've never seen the two used for the same purpose. BDD is all about your tests being especially expressive, and the distinction between "thing" (describe) and "circumstance" (context) is a part of that.

It is true that many test reporters treat them as aliases, but they really don't have to be, and if we use them the way they were intended to be used, we'd benefit from any future reporter that happened to bundle them separately. For example, if we were to ever lean on a reporter to generate API documentation for our codebase from our tests, clearly defining contexts would allow for much more interesting documentation. In any case, since tests are intentionally not coupled to reporters, I don't think we should be using that to justify using them interchangeably (or not using one at all).

Another way to think about it is that using both describe and context is a formal way to keep our tests semantic.

[epixa]

For anyone that hasn't read it, I highly recommend taking a look through https://github.com/airbnb/javascript. It's one of the most practical and thorough styleguides I've seen out there, and it seems to be consistent with all of the core bits (spacing, bracket placement, etc) that we use for kibana. It just has the added benefit of going into much more detail than our own styleguide and covers ES6 stuff.

While it won't answer all of our questions, I'd vote to outright adopt that styleguide (or another if anyone has alternative suggestions) rather than spending a ton of time trying to write and manage our own.

[tsullivan]

I agree with using describe and context to keep the structure of tests hierarchically organized and semantics. But it does mean that as reviewers, when we look at test code, we need to be clear that describe is the outer function to use, and context is the inner one. The code won't complain if we switch them.

I'm not sure if there is a lint rule for keeping the order correct- that would be ideal.

[epixa]

I don't think it's really something that could be linted since there are circumstances where the practical realities of our code may mean a describe inside a context makes sense. Consider the following scenario:

describe('#myFunc(foo)', () => {
  context('when give a foo', () => {
    it('returns a thing');
    describe('returned thing', () => {
      it('has foo as a property');
      // other tests of returned "thing"
    });
  });
});

In a typed world, the return value would have been of a specific type that we could simply test against, but in our dynamically typed world, particular in JS, we often will return object literals that are complex enough to warrant a few tests of their own but that cannot be tested as a separate standalone describe.

[w33ble]

I've run in to ES6 use with PRs that need to be backported a couple times now. We need to make a decision about how to handle that as well.

It'll be less a problem as the 4.1 branch ages and eventually leaves support, but right now it means making the changes twice, and usually writing the tests twice too.

For reference, #4613 and #4948 are the PRs I'm referring to.

[spalger]

@epixa +100 to using airbnb's style guide as a starting point.

@w33ble It's definitely worth keeping es6 backporting in mind when updating existing code to use es6. Until we move 4.2 into 4.1's spot we should probably continue to use es5 in files that are currently using es5 and stick to es6 for new development only.

@tsullivan maybe we should ask the eslint-mocha-plugin authors if they would accepts such a rule as a pr.

[w33ble]

I'm liking the "ES5 only in old code" idea, at least until 4.2 drops, possibly until it's been out for a little while (since we're presumably still going to support 4.1 for a bit).

Just had to backport #4954 by hand, that makes 3 now.

[tsullivan]

@w33ble It'd be good to integrate eslint-mocha-plugin in the first place. It has that no-exclusive-tests rule which is a good one since we kind of rely on .only to debug individual tests. They should throw a lint error just to ensure they don't get committed.

[cjcenizal]

I think this can be closed by #7435, once it's merged.

If any individual wants to re-raise discussion around a specific topic, e.g. context vs describe, then let's just create a separate ticket for the one topic, to keep discussion and PRs re the issue focused.