Add comments for assert and test interface definitions #1719

kugtong33 · 2018-02-18T08:53:04Z

Re-add deleted comments on the assert definition methods and added comments on the test interface definitions.

We can discuss the remaining definitions as I am not sure how to proceed from here.

novemberborn

I've commented with some suggestions and questions. Perhaps if you give this another pass I can then push a commit with my edits? When it comes to documentation the back and forth in PR comments can be quite frustrating for everybody involved so I don't want to make it to burdensome on you (or perhaps I'm too particular about it).

Could you share a gif like you made in #1686 that shows the descriptions? Is readability increased if you use back ticks when referring to arguments?

novemberborn · 2018-02-18T15:08:47Z

index.d.ts

 export interface Assertions {
+// Assert that actual is deep equal to expected
 deepEqual<ValueType = any>(actual: ValueType, expected: ValueType, message?: string): void;
+// Failing assertion


Perhaps "Cause the test to fail"?

novemberborn · 2018-02-18T15:09:10Z

index.d.ts

 id?: string;
 }

+// When an assert fail, the message string will be shown in the error


Would this ever be shown to the user? I agree that we shouldn't document it for each assertion though.

Using vscode 1.20.1, this is not shown, but I think it is for the better to document the message parameter on each method definition

novemberborn · 2018-02-18T15:09:37Z

index.d.ts

 fail(message?: string): void;
+// Assert that actual is equal to false
 false(actual: any, message?: string): void;
+// Assert that actual is falsy (false, 0, '', "", null, undefined, NaN)


No need to include "" in this list.

novemberborn · 2018-02-18T15:10:56Z

index.d.ts

 falsy(actual: any, message?: string): void;
+// Assert that error is falsy
 ifError(error: any, message?: string): void;
+// Assert that actual is equal to expected (use deepEqual for objects/arrays)


Perhaps "strictly equal". Using this with objects or arrays can be a valid use case so the reference to deepEqual seems distracting. There's also edge cases with negative zero and NaN but that's perhaps too detailed for inline documentation 😉

novemberborn · 2018-02-18T15:11:33Z

index.d.ts

 not<ValueType = any>(actual: ValueType, expected: ValueType, message?: string): void;
+// Assert that actual is not deep equal to expected
 notDeepEqual<ValueType = any>(actual: ValueType, expected: ValueType, message?: string): void;
+// Assert that string does not follow the regex pattern


"match" rather than "follow"

novemberborn · 2018-02-18T15:12:31Z

index.d.ts

 notDeepEqual<ValueType = any>(actual: ValueType, expected: ValueType, message?: string): void;
+// Assert that string does not follow the regex pattern
 notRegex(string: string, regex: RegExp, message?: string): void;
+// Assert that a function that never returns does not throw an error


The "never returning" part is a type detail that's not relevant to the documentation. It's so that you can write t.notThrows(() => { throw new Error() }) without type errors.

novemberborn · 2018-02-18T15:13:18Z

index.d.ts

 notThrows(value: () => ObservableLike, message?: string): Promise<void>;
+// Assert that a function that returns a promise does not throw an error
 notThrows(value: () => PromiseLike<any>, message?: string): Promise<void>;
+// Assert that a function that returns any value does not throw an error


Similarly here, returning "any value" is besides the point. Just "Assert that a function does not throw." is sufficient.

novemberborn · 2018-02-18T15:15:05Z

index.d.ts

 (title: string, macro: Macro<Context> | Macro<Context>[], ...args: Array<any>): void;
 (macro: Macro<Context> | Macro<Context>[], ...args: Array<any>): void;

+// Test hook that runs after all tests are done


Try and describe what the modifier does, rather than what it is. E.g. here "Specify hooks that run once all tests have completed". Or below, "Skip this test."

kugtong33 · 2018-02-19T09:01:51Z

@novemberborn , apologies if I didn't got it right away (that it needs to be shown in the editor somehow), I think it is better if we use jsdoc

novemberborn · 2018-02-19T10:05:36Z

I don't know… it's a bother to write and not all that readable in the tooltips. Given that the method signature is shown, referring to the variables should be sufficient IMHO. Personally I'd love to see a succinct summary of what the assertion does. Specifics can be found in the online documentation.

With the deepEqual example, actual doesn't have to be an object. It can be any value. It seems pointless to spell that out, and in any case the method signature shows it. Moreover, it makes it clear that actual and expected should have the same type. The message parameter is really hard to succinctly summarize, and I'd argue it's not even necessary to do so (except for t.throws() where it can be misleading).

kugtong33 · 2018-02-20T10:15:55Z

@novemberborn , my second take on this, missing documentation on t.log, t.skip, t.context and among other things, you can now add your edits 👍

* Refactor the `Assertion` interface so the `.skip()` modifier can be added to each assertion. * Refactor the `ExecutionContext` interface to remove top-level `.skip` and allow `.log()` and `.plan()` to be skipped. * Add error value to `.end()` in `CbExecutionContext`. * Allow plain implementations to be passed to hooks without a title * Document all assertions, execution context methods and hook & test declarations and modifiers. * Explode the `t.throws()` definitions to give better information depending on how the assertion is used. * Link to documentation where appropriate.

novemberborn · 2018-02-24T17:03:11Z

Thanks @kugtong33. I've pushed an updated TS definition. I may have gone overboard a little…

Refactor the Assertion interface so the .skip() modifier can be added to each assertion.

Refactor the ExecutionContext interface to remove top-level .skip and allow .log() and .plan() to be skipped.

Add error value to .end() in CbExecutionContext.

Allow plain implementations to be passed to hooks without a title

Document all assertions, execution context methods and hook & test declarations and modifiers.

Explode the t.throws() definitions to give better information depending on how the assertion is used.

Link to documentation where appropriate.

What do you think? I like how, when writing tests, the documentation tells you what the functions do. I suppose it reads a bit different from other API docs but I think it's more useful for AVA.

How did you do your screen recordings?

I haven't updated the Flow definition yet, and the changes I made mean it's a bit trickier to copy things across. I feel I can barely ask if you'd be up for doing that… but are you? 😄

kugtong33 · 2018-02-26T01:59:03Z

@novemberborn , absolutely yes, I will look into the new updates and implement it on the flow definitions

I am using peek for the screen recordings

kugtong33 · 2018-02-28T09:54:07Z

@novemberborn

Is it necessary to restructure the flow definitions, that it would closely resemble the typescript definitions?

novemberborn · 2018-02-28T20:44:07Z

@kugtong33 that would be ideal. It makes it easier to update both files when we add features, since you can diff them against each other and see where you need to make changes.

kugtong33 · 2018-03-04T11:50:36Z

@novemberborn

Restructured flow definition with what I understood, certain definitions that I directly copied from the ts file doesn't work with flow, but I tried it to be as close as possible.

Will add screen captures tomorrow

P.S.
I see that tests for flow and typescript definitions are only done when there is a need to regress, what if we add dedicated tests, is it possible?

novemberborn · 2018-03-06T16:20:38Z

Wow great work @kugtong33! I've previously had trouble getting Flow to work well for me. If you can see the right descriptions then I'm happy. I did push one minor tweak.

I see that tests for flow and typescript definitions are only done when there is a need to regress, what if we add dedicated tests, is it possible?

That'd be great to have. It's just a lot of use cases to write 😄

kugtong33 · 2018-03-08T00:17:25Z

@novemberborn

When you select from the existing methods

When you write the method name with parenthesis

novemberborn · 2018-03-08T15:27:13Z

When you select from the existing methods

…

When you write the method name with parenthesis

That difference is fine I think. I tried test.serial.after.always(() => {}) in VS Code and it showed the right information when hovering each part of that chain. I'm going to land this, thanks @kugtong33!

Add comments for assert and test interface definitions

ed4dcba

kugtong33 force-pushed the typedef-embedded-documentation branch from cca9e69 to ed4dcba Compare February 18, 2018 09:31

novemberborn requested changes Feb 18, 2018

View reviewed changes

kugtong33 force-pushed the typedef-embedded-documentation branch from 71f9e60 to 56fd1f1 Compare February 19, 2018 08:51

kugtong33 force-pushed the typedef-embedded-documentation branch from 56fd1f1 to 6bfe9ba Compare February 19, 2018 14:08

kugtong33 added 2 commits February 20, 2018 17:47

Use jsdoc for embedded documentation on typedefs

b39bf55

Merge branch master into PR-1719

363a004

kugtong33 force-pushed the typedef-embedded-documentation branch from 6bfe9ba to 363a004 Compare February 20, 2018 09:51

Add missing param highlights

9c5c91c

novemberborn added 2 commits February 24, 2018 15:16

Merge branch 'master' into pr/1719

e2575da

[WIP] Embedded docs on flow definition

0bf0bfb

kugtong33 added 2 commits March 4, 2018 19:41

Restructure flow file, that it closely follows the d.ts file

6d0d206

Merge branch master into PR-1719

4375afc

Tweak Flow SnapshotOptions definition

b63f4b5

novemberborn merged commit ecc219a into avajs:master Mar 8, 2018

kugtong33 deleted the typedef-embedded-documentation branch March 9, 2018 01:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add comments for assert and test interface definitions #1719

Add comments for assert and test interface definitions #1719

Uh oh!

kugtong33 commented Feb 18, 2018

novemberborn left a comment

novemberborn Feb 18, 2018

novemberborn Feb 18, 2018

kugtong33 Feb 19, 2018

novemberborn Feb 18, 2018

novemberborn Feb 18, 2018

novemberborn Feb 18, 2018

novemberborn Feb 18, 2018

novemberborn Feb 18, 2018

novemberborn Feb 18, 2018

kugtong33 commented Feb 19, 2018

novemberborn commented Feb 19, 2018

kugtong33 commented Feb 20, 2018 •

edited

Loading

novemberborn commented Feb 24, 2018

kugtong33 commented Feb 26, 2018

kugtong33 commented Feb 28, 2018 •

edited

Loading

novemberborn commented Feb 28, 2018

kugtong33 commented Mar 4, 2018 •

edited

Loading

novemberborn commented Mar 6, 2018

kugtong33 commented Mar 8, 2018 •

edited

Loading

novemberborn commented Mar 8, 2018

Labels

2 participants

Add comments for assert and test interface definitions #1719

Add comments for assert and test interface definitions #1719

Uh oh!

Conversation

kugtong33 commented Feb 18, 2018

novemberborn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kugtong33 commented Feb 19, 2018

novemberborn commented Feb 19, 2018

kugtong33 commented Feb 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

novemberborn commented Feb 24, 2018

kugtong33 commented Feb 26, 2018

kugtong33 commented Feb 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

novemberborn commented Feb 28, 2018

kugtong33 commented Mar 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

novemberborn commented Mar 6, 2018

kugtong33 commented Mar 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

novemberborn commented Mar 8, 2018

Labels

2 participants

kugtong33 commented Feb 20, 2018 •

edited

Loading

kugtong33 commented Feb 28, 2018 •

edited

Loading

kugtong33 commented Mar 4, 2018 •

edited

Loading

kugtong33 commented Mar 8, 2018 •

edited

Loading