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

Add docs for test parameter need to be named t #1096

Merged
merged 3 commits into from
Nov 4, 2016
Merged

Add docs for test parameter need to be named t #1096

merged 3 commits into from
Nov 4, 2016

Conversation

gurpreetatwal
Copy link
Contributor

  • Reworded the statement in the readme regarding the first parameter needing to be t to make it stronger and explain why its required.
  • Add a note in common-pitfalls regarding the same issue
  • Make the indentation style consistent in the code samples

Closes #1031

@@ -63,6 +63,16 @@ test(async t => {

AVA [can't trace uncaught exceptions](https://github.com/avajs/ava/issues/214) back to the test that triggered them. Callback-taking functions may lead to uncaught exceptions that can then be hard to debug. Consider promisifying and using `async`/`await`, as in the above example. This should allow AVA to catch the exception and attribute it to the correct test.

### Why is the power-assert information not shown?

Ensure that the first parameter passed into your test is named `t`. This is a byproduct of the way power-assert works. It uses a pattern matching scheme that makes it easier for implementors to wrap any assertion library with power-assert goodness without having to understand the ES AST at all. See [#1031](https://github.com/avajs/ava/issues/1031) for more details.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a byproduct of the way power-assert works.

I'd rather say that "This is a requirement by power-assert.".

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 change this to:

Why are the enhanced assertion messages not shown?

Ensure that the first parameter passed into your test is named t. This is a requirement of how AVA uses power-assert, the library that provides the enhanced messages. works.

I see that this paragraph originally comes from @jamestalmage's comment in #1031 (comment). It's quite technical though. I don't think we need that kind of detail here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fully agree, that was just some blind copy and pasting from the comment on my part. Should I still keep the link to the issue for anyone who wants to know more?

Copy link
Member

Choose a reason for hiding this comment

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

Nah, the issue still requires a lot of careful reading and background to fully understand why it is this way.

@vadimdemedes
Copy link
Contributor

Good job explaining the what & why!

Copy link
Member

@novemberborn novemberborn left a comment

Choose a reason for hiding this comment

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

Thanks @gurpreetatwal! I feel the explanation is unnecessarily technical though. It's interesting background information, sure, but people just want to know how to make their test setup work.

Make the indentation style consistent in the code samples

I'm going to have to ask you to revert this, sorry. package.json files require two-space indentation. (You could change it, but the next time npm writes that file it forces its indentation on you anyway.)

@@ -63,6 +63,16 @@ test(async t => {

AVA [can't trace uncaught exceptions](https://github.com/avajs/ava/issues/214) back to the test that triggered them. Callback-taking functions may lead to uncaught exceptions that can then be hard to debug. Consider promisifying and using `async`/`await`, as in the above example. This should allow AVA to catch the exception and attribute it to the correct test.

### Why is the power-assert information not shown?

Ensure that the first parameter passed into your test is named `t`. This is a byproduct of the way power-assert works. It uses a pattern matching scheme that makes it easier for implementors to wrap any assertion library with power-assert goodness without having to understand the ES AST at all. See [#1031](https://github.com/avajs/ava/issues/1031) for more details.
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 change this to:

Why are the enhanced assertion messages not shown?

Ensure that the first parameter passed into your test is named t. This is a requirement of how AVA uses power-assert, the library that provides the enhanced messages. works.

I see that this paragraph originally comes from @jamestalmage's comment in #1031 (comment). It's quite technical though. I don't think we need that kind of detail here.

```js
test(t => {
t.is(1, 1);
}
Copy link
Member

Choose a reason for hiding this comment

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

Missing ); at the end here.

},
"devDependencies": {
"ava": "^0.15.0"
}
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 revert these to their original two space indentation? It's how package.json files are formatted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah okay, did not know npm did that. I'll change those back. There are two other json snippets (L664 and L680) that are currently indented with tabs, should I change those as well?

Copy link
Member

Choose a reason for hiding this comment

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

Yes. There should be no unrelated whitespace changes in this PR.

@sindresorhus sindresorhus changed the title Add docs for test parameter need to be named t Add docs for test parameter need to be named t Nov 4, 2016
@sindresorhus sindresorhus merged commit 3c5b685 into avajs:master Nov 4, 2016
@sindresorhus
Copy link
Member

Thanks :)

@gurpreetatwal gurpreetatwal deleted the t-docs branch November 4, 2016 05:42
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.

4 participants