New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document common pitfalls #404

Closed
jamestalmage opened this Issue Jan 3, 2016 · 30 comments

Comments

Projects
None yet
9 participants
@jamestalmage
Contributor

jamestalmage commented Jan 3, 2016

There are some points that continue to confuse people (especially as it relates to our async behavior).

One in particular is using closure scope variables to try and pass data between beforeEach and individual tests (see #402).

I think we need to create some documentation that addresses each of these types misconceptions so we can quickly link to them. They can be in the readme if they are concise, but maybe separate documents if they require lengthy explanations.

If anyone can think of other common downfalls / misconceptions / misuses - go ahead and add them to this discussion.

@ariporad

This comment has been minimized.

Contributor

ariporad commented Jan 3, 2016

I think this is a really great idea. Also, I'd like to appreciate the relevant issue number.

@sindresorhus

This comment has been minimized.

Member

sindresorhus commented Jan 3, 2016

Good idea!

@sindresorhus

This comment has been minimized.

Member

sindresorhus commented Jan 3, 2016

Pitfalls:

  • Using global variables and forgetting that tests run concurrently.
  • console.log not logging where the test result is printed (I hope we can fix #392, but we should document until we do)
@ariporad

This comment has been minimized.

Contributor

ariporad commented Jan 3, 2016

Actually, What if we made this a wiki page and linked to it from the README? That way others could contribute?

@sindresorhus

This comment has been minimized.

Member

sindresorhus commented Jan 3, 2016

What if we made this a wiki page and linked to it from the README?

No. I want to have quality control on the docs. I don't want random strangers to be able to just freely modify the docs. Also: https://plus.google.com/+sindresorhus/posts/QSS2du26Mg4

That way others could contribute?

They already can with a pull request.

@novemberborn novemberborn changed the title from Document some common downfalls. to Document some common pitfalls. Apr 5, 2016

@novemberborn

This comment has been minimized.

Member

novemberborn commented Apr 13, 2016

Another one: if you run AVA in Docker as part of your CI, you need to forward the appropriate environment variables (see https://github.com/watson/is-ci/blob/master/index.js), or configure the verbose reporter. See #751.

@sotojuan

This comment has been minimized.

Contributor

sotojuan commented Jun 8, 2016

Should we make a common_pitfalls.md in the documentation directory with these?

@jamestalmage

This comment has been minimized.

Contributor

jamestalmage commented Jun 8, 2016

yep

@sindresorhus

This comment has been minimized.

Member

sindresorhus commented Jun 9, 2016

👍

common_pitfalls.md => common-pitfalls.md

@sotojuan sotojuan self-assigned this Jun 9, 2016

@sindresorhus

This comment has been minimized.

Member

sindresorhus commented Jun 17, 2016

Another pitfall I see often is trying to do an async operation in a normal test and not understand why it's not finishing.

Forgetting to return the promise:

test(t => {
  fetch().then(data => {
    t.is(data, 'foo');
  });
});

Or should have used a callback test:

test(t => {
  fetch((err, data) => {
    t.is(data, 'bar');
  });
});
@novemberborn

This comment has been minimized.

@sindresorhus sindresorhus changed the title from Document some common pitfalls. to Document common pitfalls Jul 1, 2016

@sindresorhus

This comment has been minimized.

Member

sindresorhus commented Jul 1, 2016

Now documented here: https://github.com/avajs/ava/blob/master/docs/common-pitfalls.md

Gonna keep this issue open for discovery reasons.

@sindresorhus sindresorhus reopened this Jul 1, 2016

@sindresorhus

This comment has been minimized.

Member

sindresorhus commented Aug 4, 2016

I've seen people trying to pass CLI flags to their tests: $ ava --user-flag This doesn't work as we don't pass flags to the test processes. Instead, users should use environment variables.

@jfmengels

This comment has been minimized.

Contributor

jfmengels commented Aug 8, 2016

Should we also link to eslint-plugin-ava in the common pitfalls section? We can probably push more people to using it.

@sindresorhus

This comment has been minimized.

Member

sindresorhus commented Aug 8, 2016

@jfmengels Good idea! Can you add it?

@jfmengels

This comment has been minimized.

Contributor

jfmengels commented Aug 8, 2016

Sure, will do that later.

@sotojuan

This comment has been minimized.

Contributor

sotojuan commented Aug 30, 2016

What abut source vs files in the AVA package.json options?

@sindresorhus

This comment has been minimized.

Member

sindresorhus commented Aug 30, 2016

@sotojuan Have you encountered anyone getting confused by those?

@sotojuan

This comment has been minimized.

Contributor

sotojuan commented Aug 30, 2016

Just did on an IRC!

@sindresorhus

This comment has been minimized.

Member

sindresorhus commented Aug 30, 2016

Can you copy paste the conversation for context? Like, what was confusing about it.

@sotojuan

This comment has been minimized.

Contributor

sotojuan commented Aug 30, 2016

Ah, sorry I should've just said everything in the first comment. My friend was just confused about the difference between both (what do they do?)—they do look very similar. Maybe not a pitfall but worth a sentence or two in the readme.

@sindresorhus

This comment has been minimized.

Member

sindresorhus commented Sep 1, 2016

@sotojuan Yeah, definitely. If anything is unclear, we should make it clearer. Wanna do a PR improving it?

@cameronroe

This comment has been minimized.

cameronroe commented Nov 25, 2016

When import ing a sass file with webpack and ava, the import fails. Any way to fix that?

@sindresorhus

This comment has been minimized.

Member

sindresorhus commented Nov 25, 2016

@cameronroe http://stackoverflow.com/questions/tagged/ava would be a better place to ask this. Include some more info too, like AVA and Webpack config.

@davewasmer

This comment has been minimized.

davewasmer commented Nov 28, 2016

Not sure if I should add this here, or file a separate issue, but one pitfall I ran into:

If your tests do anything memory intensive, you might end up hitting OOM process killers in CI environments because ava runs them in parallel. See this build as an example.

@cameronroe

This comment has been minimized.

cameronroe commented Nov 29, 2016

@sindresorhus okay, so no one is alive on Stack overflow.. Any ideas on what's up with ava and webpack? Docs anywhere on the subject?

@sindresorhus

This comment has been minimized.

Member

sindresorhus commented Nov 29, 2016

@sindresorhus

This comment has been minimized.

Member

sindresorhus commented Nov 29, 2016

@cameronroe You didn't include the information I recommended. Just a guess, but you need to use https://github.com/istarkov/babel-plugin-webpack-loaders or precompile your component before testing.

@ProLoser

This comment has been minimized.

ProLoser commented Nov 6, 2017

Would love to have a pitfall around the parallel nature of unit tests. I can't tell you how many times I've run into Sinon throwing errors about methods already being wrapped in spies. It's just not something I expected to encounter.

Specifically, illustrating the workflow of before, beforeEach, test, afterEach, after and the fact that beforeEach may run multiple times in a row. I realize the answer has been 'make your tests serial' it's just not something I expect to run into. At least illustrating that if you have to mess with singletons or global objects or static properties, that beforeEach can cause issues.

One thing I'm still not clear on: if I want to write my tests in a parallel way, are the tests run concurrently, or should I be putting the global modifications and subsequent cleanup into the test itself? In which case, running things like sinon.sandbox.restore() can't be done in an afterEach()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment