Oni: Add more tests

[ghost]

Couldn't find an issue directly addressing this, but it would be pretty nice, if not essential, that there's a good testing framework for making sure that changes do not regress the code. I understand that while the code is still in flux testing does not matter that much, but given that there is already quite a rich and working feature set, it'd be a shame if it became buggy.

Personally, I think Jest is the best testing framework in this context because of its integration with React components. Have to admit I haven't used it myself, though.

[bryphe]

Definitely! Thanks for logging the issue, @samvv !

We're missing a lot of coverage today, and that won't scale out as more and more people start contributing to the project.

I understand that while the code is still in flux testing does not matter that much, but given that there is already quite a rich and working feature set, it'd be a shame if it became buggy.

💯 - this is a very important point. It's tough to win users over mature editors like Atom/VSCode/Vim/Emacsif they don't feel like they can trust our quality. Having tests is our safety net to validate things stay working build-over-build and release-over-release.

We don't have a very good documentation, but here's the sort of testing we have today:

• __Unit Tests__ - these live in browser/test. An example of one is:
  https://github.com/onivim/oni/blob/68671386ec749b4e3f25ce71695c03fdccd697e7/browser/test/Services/Completion/CompletionTests.ts#L72

You can run these via npm run test:unit - it's a relatively small number today. This would be a great area to focus on!

• '__Ci Tests'__ - these are integration/end-to-end tests that exercise a running instance of Oni. Most of the interaction occurs via the oni-api surface area. These tests live in test/ci, and an example of one here is:
  https://github.com/onivim/oni/blob/68671386ec749b4e3f25ce71695c03fdccd697e7/test/ci/LargeFileTest.ts#L11

Both of these currently use mocha, but I'm open to change as long as it's low friction.

In both cases, it would be excellent to have more coverage. IMO unit tests are always preferred, as they tend to be more reliable/less moving parts - but in some cases, we need the end-to-end coverage, especially when validating against Neovim.

For unit testing, I think as we go, there'll be some refactoring work too to make the code testable.

Code coverage numbers would also be useful, it's not a perfect metric but it can help us quantify how much of our code is under test (how much progress we're making), and where gaps may be.

[bryphe]

Also, perhaps as the project + our test framework mature, we might want to add test coverage as a requirement for PRs.

[ghost]

Also, perhaps as the project + our test framework mature, we might want to add test coverage as a requirement for PRs.

Yup, sounds like a good idea to me. And thanks for the information. Will look into it when I find some time.

[ghost]

@Bretley I've been trying out some testing frameworks and I really think the best option would be AVA in combination with Enzyme, contrary to what I first said. With it, tests should complete _really_ fast as ava supports full parallelism. Jest is does not provide as much testing integration with React as it hoped it would, and it seems it does not support parallelism like ava does

I can set it up and convert existing tests if you want. Also added some tests for some additional methods in NeovimInstance that I'm experimenting with. Like you predicted I needed to make some changes to the internal API and decouple some components so that they can be easily tested. Will keep you updated and create a pull request as soon as possible so you can see for yourself.

[akinsho]

@bryphe I'd like to help out more with some of the testing but personally am completely unfamiliar with ava and somewhat daunted by the ciTests in general, where I'd like to start is with helping to set up more unit tests for some of Oni's components using enzyme which was being discussed here although I appreciate that hasn't been implemented.

If you're still amenable to using enzyme (which I'm much more familiar with for testing react) I can put together some tests for some of our ui components? maybe 😟 If I start to feel a bit less daunted down the line I could look at some of what was suggested here re using Ava although I'd definitely prefer not to rush in with that.

[bryphe]

I'd like to help out more with some of the testing but personally am completely unfamiliar with ava and somewhat daunted by the ciTests in general

Awesome! Would be great to have help 😄 I think it makes sense to split up the work - switching the test framework to ava _and_ introducing enzyme sounds intense! And they are basically orthogonal efforts, because my understanding is that enzyme works fine with our existing mocha strategy, too.

I'd like to start is with helping to set up more unit tests for some of Oni's components using enzyme which was being discussed here although I appreciate that hasn't been implemented.

Sounds like an excellent way to start. We don't have much (any?) testing at all of our React components, so it'd be very helpful to have a precedent in place and examples. I'm definitely on board with using enzyme for testing our React components. Let me know if you need any help getting started.

[akinsho]

@bryphe wanting to take you up on help re setting up enzyme or rather issues I'm having re typescript reading react in the test dir. At the moment I've created a basic test at /browser/test/components with a separate command for these tests as mocha needs different arguments for this but the tsc in this directory seems to not recognise react which is confusing because the tsconfig reads

{
    "compilerOptions": {
        "allowUnreachableCode": false,
        "allowUnusedLabels": false,
        "experimentalDecorators": true,
        "forceConsistentCasingInFileNames": true,
        "jsx": "react",
        "lib": ["dom", "es2017"],
        "module": "commonjs",
        "moduleResolution": "node",
        "newLine": "LF",
        "noEmitOnError": true,
        "noFallthroughCasesInSwitch": true,
        "noImplicitAny": true,
        "noImplicitReturns": true,
        "noImplicitThis": true,
        "noUnusedLocals": true,
        "outDir": "../lib_test/browser",
        "pretty": true,
        "removeComments": true,
        "rootDir": ".",
        "skipLibCheck": true,
        "strictNullChecks": false,
        "suppressImplicitAnyIndexErrors": true,
        "target": "es2015",
        "sourceMap": true
    },
    "include": ["src/**/*.ts", "test/**/*.ts", "browser/test/**/*.tsx"],
    "exclude": ["node_modules"]
}

Any ideas or clues why re the setup of our dirs tsx in those files is causing issues?

[akinsho]

Whoops nevermind, silly mistake on my part the file was named .ts not .tsx........................ :bow:
Although whilst I'm here I wonder how open you'd be to switching to a different testing tool, I know most ppl associate jest with enzyme and react but it's actually a fully fledged testing framework that offers mocking, spying, assertion, snapshot testing as well I think as similar parallelization as ava maybe not the same but think it does.. (need to crosscheck that) It provides a much nicer testing experience allowing you to watch tests only running tests either by regex or on only files most recently changes loads of features ✨. It also offers a code mod for migration so can do a lot of the heavy (not all Im sure of switching over)? obv. no pressure either way just given we discussed that sort of thing here before thought i'd raise it

[bryphe]

Although whilst I'm here I wonder how open you'd be to switching to a different testing tool, I know most ppl associate jest with enzyme and react but it's actually a fully fledged testing framework that offers mocking, spying, assertion, snapshot testing as well I think as similar parallelization as ava maybe not the same but think it does.

I'm open to another testing tool! I have no strong tie to mocha, it just was easy to set up and get going. Some things to consider:

• If we decide that we want to go that route, can we make the change _incrementally_? For example - perhaps start by using jest+enzyme with react components, and then incrementally migrating existing tests over in batches. Or just starting by integrating enzyme, and then look at jest as a separate change?
• Following from the _incremental_ change, there needs to be a concrete migration plan for moving from our existing test cases to the new story.
• We need to make sure we have a story for debugging (like npm run debug:test:unit:browser).
• We need to make sure the code coverage functionality is preserved (some special work had to be done in terms of outputting coverage / modifying require) with the new testing strategy.

As long as we have a clear, concrete plan for those 4 bullet points, I'm okay with a change!

[akinsho]

@bryphe all very reasonable, tbh I'm not looking to do this imminently I was just having quite a lot of trouble using mocha with enzyme, so whilst it is possible the mixture of typescript, mocha and enzyme configs meant things kept imploding. I've taken of the above suggestions the one to localize enzyme and jest to react as jest has much simpler tools for working with typescript which meant that things just worked. I'm going to make a PR soon for the initial setup with just a single test file then I can build on that with subsequent PRs as the infrastructure is in place

[bryphe]

I'm going to make a PR soon for the initial setup with just a single test file then I can build on that with subsequent PRs as the infrastructure is in place

@Akin909 - that sounds great!