Skip to content
This repository has been archived by the owner on Aug 5, 2020. It is now read-only.

Refactor Test Suite inside GlueStick project #406

Closed
toddw opened this issue Jan 12, 2017 · 19 comments
Closed

Refactor Test Suite inside GlueStick project #406

toddw opened this issue Jan 12, 2017 · 19 comments
Assignees

Comments

@toddw
Copy link
Contributor

toddw commented Jan 12, 2017

There are a few issues with the current test suite:

  1. Debugging tests is currently broken. We would like to use https://nodejs.org/api/debugger.html#debugger_v8_inspector_integration_for_node_js
  2. We no longer use Karma, all Karma stuff should be removed.
  3. It feels as though the additional spawn might be unnecessary although it was said to have been added for speed.
  4. Decouple coverage reports from running every time a test is re-run.
@threehams
Copy link
Collaborator

The spawn with --watch was necessary at the time buuuuut... is definitely not the best solution, especially since --watch is being deprecated.

Any solution that allows incremental compilation would work here. Mocha 2 has a complicated and undocumented programmatic method for running tests. If Mocha 3 has improved that, and we can use chokidar to watch files and programmatically re-run tests with incremental compilation, that'd be the best of everything.

If we just restart mocha each time, we're looking at 12-15 seconds on every file change before tests even run. This is what caused us to add an alternative test runner into the app to get back to 10ms.

We do have to be careful not to run tests and the server in the same process, or wacky stuff happens. Been there before.

@toddw
Copy link
Contributor Author

toddw commented Jan 12, 2017

It would be nice to be able to take advantage of mocha's many options straight from the CLI. For example, the --grep option for selecting a set of tests to run.

@threehams
Copy link
Collaborator

I agree, but Mocha's forcing our hand a bit on that one by getting rid of --watch. I'll try to find the discussion a little later if you need it for reference.

@toddw
Copy link
Contributor Author

toddw commented Jan 12, 2017

Maybe then, it would be worth considering Jest instead?

@toddw
Copy link
Contributor Author

toddw commented Jan 12, 2017

It may not be so painful to convert to Jest with things like https://github.com/skovhus/jest-codemods

When we started GlueStick, there was a lot of talk about Jest looking like it was basically abandonware… The Jest I'm seeing today looks incredibly different than before.

My favorite items on the features list

  1. Fast interactive mode with --watch (yay!)
  2. Jest works with any compile-to-JS language and integrates seamlessly with Babel.
  3. Create coverage reports with --coverage. No additional setup or libraries needed! (maybe we wont have istanbul problems anymore)
  4. Run tests in parallel processes to minimize test runtime.

Some of these might overlap with Mocha but those might be a lot cleaner/simpler/faster?

@threehams
Copy link
Collaborator

Relevant discussion:
mochajs/mocha#1780 (comment)

@threehams
Copy link
Collaborator

If another test runner produces faster and more reliable results, great! Mocha was a great choice at first because of the incredible speed of the thing (10ms from test run to pass/fail). Once a project has 1000+ tests, running in parallel may help a lot.

@ferrannp
Copy link
Contributor

ferrannp commented Jan 18, 2017

I am on the Jest side in here. Moreover, I'd like to discuss different approach if we move to Jest on folder structure. Right now I see we have test folder which replicates structure of src. It seems kinda repetitive (see test/templates...) and if something changes in src, we need to change in test too. I'd go with:

src/
  autoUpgrade /
    __tests__ /
 commands /
   __tests__ /
 ...

Jest will look into all __tests__ folder and run the tests inside of it (everything inside __tests__ is consider a test). Right now there are cases that can be confusing like test/fixtures/projectPackages.js which is not actually a test but is inside the test folder.

A part from benefits mentioned in here, you can also:

  • Use Snapshots if you are interested on it
  • Run single test with: npm test -- testName
  • As mentioned use npm test -- --watch and it has the option (by default) to re-run only the related to changed files
  • Super nice CLI and errors like (plus colors):
  expect(jest.fn()).toHaveBeenCalledTimes(1)
  Expected mock function to have been called one time, but it was called two times.

It might take a bit more of work but I can refactor in a way we do not need sinon, chai, etc. Just one dependency, jest.

Same for Coverage #345 which can be run with npm test -- --coverage and won't require istanbul.

@threehams
Copy link
Collaborator

I would argue for having tests live alongside source files regardless of whether we go with Jest or not - I go as far as putting source.js and source.spec.js in the same folder. There's nothing about Mocha that requires a separate test directory.

@toddw
Copy link
Contributor Author

toddw commented Jan 19, 2017

@ferrannp @threehams tests were put into their own folder specifically to mimic Rails… however, I would much rather follow whatever the popular convention for Jest is if we move that direction. We should consider what this means for existing projects that have tests in the tests folder right now… I suppose someone could manually move them, would it be a lot of work to support the current folder structure for older apps with a deprecation warning? If that creates a ton of work or makes the code a lot messier then I would prefer we just make this a breaking change with GlueStick 1.0

Same for Coverage #345 which can be run with npm test -- --coverage and won't require istanbul

👍 stoked about that

I'm on board with moving to Jest

@threehams
Copy link
Collaborator

I'm on board with the best option available, provided it gives quick test results (startup time is much less important). I can't stress enough how much <250ms test results help when using TDD in our current project.

@zamotany
Copy link
Contributor

Debugging tests is currently broken. We would like to use https://nodejs.org/api/debugger.html#debugger_v8_inspector_integration_for_node_js

I believe that's not possible, since tests files are not shown in chrome inspector so it's impossible to set breakpoints in it, also debugger; statements are omitted in chome dev tools. This is actually logical, since by the time node starts debugging, node is unaware of those test files - they are required after Jest module is loaded and it's setup is done. In this case the only option to debug tests is to run node debugger (not inspector) and connect to it. VS Code have built in debugger that works just fine, I believe WebStorm also, I don't know if Atom has support for node debugger but I suppose there is an extension for that.

@sethbattin
Copy link
Contributor

@zamotany can you elaborate why inspector won't work? It's simply inspecting a node process running elsewhere, so it works identically to vscode, webstorm, etc. It shouldn't have any specific limitation on whatever system executes the test js, even with dynamic requires. Also afaik debugger statements work just fine in dev tools, so I must be missing some context about what you mean about that.

@zamotany
Copy link
Contributor

@sethbattin

It's simply inspecting a node process running elsewhere, so it works identically to vscode, webstorm, etc.

Node debugger and Chrome Dev Tools have different communication protocols. VS Code/WebStorm/Atom use Node Debugger Protocol.

Also afaik debugger statements work just fine in dev tools

debugger statements works in Chrome Dev Tools if they are set in code that runs in browser. Here it runs on node.

I couldn't manage to debug Jest running with node --inspect --debug-brk ./node_modules/.bin/jest -i and inspect it with Chrome Dev Tools - I had to skip lines to get test files loaded (it took ~10min).
Also to set breakpoint in Chrome Dev Tools file have to listed in Sources tab, but when node starts debugging test files are not included. Moreover if --debug-brk is removed test won't break at debugger statements at all.

@sethbattin
Copy link
Contributor

I had to prove it to myself; I'm pretty sure there's no difference between dev tools and other debugging options, regardless of communication protocol. The debugging is remotely observing a node process in either case, and the handling of the debugging is a matter of tooling rather than comm protocol.

Here are the current mocha tests for a generated gluestick app, being debugged with node --inspect, and hitting a debugger line in the test scripts:
image

I launched this debugging session from gitbash with this command:

NODE_ENV=development-test "C:\Program Files\nodejs\node.exe" --inspect --debug-brk=60823 --expose_debug_as=v8debug -r ../gluestick/src/l
ib/testHelperMocha.js C:\\Users\\sbattin\\AppData\\Roaming\\npm\\node_modules\\mocha\\bin\\_mocha --compilers js:babel-core/register --tim
eout 0 --ui bdd --reporter "C:\Program Files (x86)\JetBrains\WebStorm 2016.3.2\plugins\NodeJS\js\mocha-intellij\lib\mochaIntellijReporter.
js" C:\\Users\\sbattin\\checkouts\\react-boot-example\\test\\components\\Boot.test.js

This command is a slightly modified version of what webstorm issues when starting debugging, which is in turn something I extracted from the process spawned by gluestick test -S


However, my test obviously shows the transpile output being debugged. This has caused issued in webstorm debugging due to lack of source maps (execution line mismatch, variable existence, etc). That might explain why debugger statements appear to be nonfunctional: they aren't lining up with actual code execution. Webstorm's help article suggest adding source maps in order to get proper debugging, and i suspect it would be helpful to chrome debugging as well.

@sethbattin
Copy link
Contributor

And now I see I was too hasty in capturing my screenshot, because Chrome was offering me source maps. Webstorm's problems are its own, I suppose..

Here's the pretty-printed version, also with breakpoints:
image

@ferrannp
Copy link
Contributor

@sethbattin you might wanna try debugging in the PR I sent #437 and share your thoughts about it too ? :)

@sethbattin
Copy link
Contributor

will do

@zamotany
Copy link
Contributor

Finally I managed to break Jest test on debugger; statement.
Here is a reference to original issue: jestjs/jest#1652

To make everything work proper env should be passed:

node --inspect --debug-brk ./node_modules/.bin/jest -i --env jest-environment-node-debug test.spec.js

@toddw toddw changed the title Refactor Test Suite Refactor Test Suite inside GlueStick project Jan 24, 2017
@toddw toddw closed this as completed Jan 24, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
No open projects
Development

No branches or pull requests

5 participants