Skip to content
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 #919

Merged
merged 15 commits into from
Jul 1, 2016
Merged

Document common pitfalls #919

merged 15 commits into from
Jul 1, 2016

Conversation

sotojuan
Copy link
Contributor

Fixes #404.

I added @novemberborn's Docker idea.

I'm looking for more pitfalls and what I can write about them. Here are some from the issue:

Using global variables and forgetting that tests run concurrently

Does saying "AVA run test files in different processes so there's no shared global state..." suffice?

console.log not logging where the test result is printed

What's the status on this with the closed issue (#392)?

Any other ideas welcome, this can be WIP until we have a good amount.


If you run AVA in Docker as part of your CI, you need to fix the appropriate environment variables. Specifically, adding `-e CI=true` in the `docker exec` command. See [https://github.com/avajs/ava/issues/751](#751).

AVA uses [is-ci](https://github.com/watson/is-ci) to decide if it's in a CI environment or not using [these](https://github.com/watson/is-ci/blob/master/index.js) variables.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AVA uses is-ci to decide whether it's in a CI environment using these environment variables.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe link these variables? It makes the link more descriptive.

@sindresorhus
Copy link
Member

sindresorhus commented Jun 17, 2016

Good start.

Needs to be linked to from the readme.


## AVA and connected client limits

You may be using a service that only allows a limited number of concurrent connections. For example, many database-as-a-service businesses offer a free plan with a limit on how many clients can be using it at the same time. AVA can hit those limits and cause the tests to hang because it runs multiple processes, each of which is a different client.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should be clear that it's those services that are being bad and not AVA. They shouldn't hang AVA just because it's concurrency greedy.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well written clients / services should emit errors if concurrency limits are exceeded, instead of transparently hanging execution. If they don't, you should consider submitting an issue request asking that they do.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jamestalmage No, they should just handle it with throttling, but very few do...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IDK, the whole point seems a little unnecessary. Services are going to do what they do, we are just explaining the reality of how to make AVA work with services that behave this way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. I just want to make it clear they're working around the service, not AVA, when they follow this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@sotojuan
Copy link
Contributor Author

Updated.

@novemberborn
Copy link
Member

Maybe add a call to action for people reading this document and thinking "but my problem isn't listed here"?

@sindresorhus
Copy link
Member

@sotojuan Interested in including something about #404 (comment) here, or would you prefer doing that in a separate PR?

@sotojuan
Copy link
Contributor Author

I'll add it! We can close the PR when we think we have enough to get started. Will write it up after I finish breakfast.

@sotojuan
Copy link
Contributor Author

Updated.


## Async operations

You may be running an async operation inside a test and wondering why it's not finishing. If your async operation uses promises, you should be returning the promise:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"you should return the promise"

@sotojuan
Copy link
Contributor Author

🐔

@sindresorhus
Copy link
Member

Needs to be linked to from the readme.

@sotojuan
Copy link
Contributor Author

Got it—put it in Support if that's a good place.

@novemberborn
Copy link
Member

@sotojuan probably better under Tips.

@sotojuan
Copy link
Contributor Author

Updated.

"devDependencies": {
"ava": "^0.11.0"
}
"name": "awesome-package",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you leave the whitespace as it was?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was part of the merge conflict (this part was an old version from whenever I made my fork). Fixed when I merged readme from master.

"babel-register"
],
"babel": "inherit"
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't do unrelated changes.

@sotojuan
Copy link
Contributor Author

Fixed all the docs stuff—sorry about the unrelated commits (still not good at merge conflicts...), how would I fix that? Rebase?

@sindresorhus
Copy link
Member

@sotojuan Doesn't matter. We squash on merge, so it's not included. You still have to fix the indentation changes in the readme though; https://github.com/avajs/ava/pull/919/files#diff-0730bb7c2e8f9ea2438b52e419dd86c9R163

@sotojuan
Copy link
Contributor Author

All right, sorry about that, I did that commit in my work computer and probably didn't have editorconfig. Should be better now (?).

@sindresorhus sindresorhus changed the title [WIP] Document common pitfalls Document common pitfalls Jul 1, 2016
@sindresorhus sindresorhus merged commit 187b8b4 into avajs:master Jul 1, 2016
@sindresorhus
Copy link
Member

Landing this now. We can fill in more as pitfalls are discovered. Awesome work @sotojuan :)

@sotojuan sotojuan deleted the common-pitfalls branch July 1, 2016 00:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document common pitfalls
4 participants