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

docs(spec): show tests in ESDocs #1378

Merged
merged 1 commit into from
Mar 4, 2016
Merged

Conversation

staltz
Copy link
Member

@staltz staltz commented Feb 23, 2016

Add ESDoc @test tag on each *-spec.js file in order to make it show in the ESDoc page, and also
tweak esdoc.json to show tests.

To see this page live, open http://rxjs5-esdoc-tests.surge.sh/test.html

screen shot 2016-02-23 at 11 19 12

@@ -2,6 +2,7 @@
var Rx = require('../../dist/cjs/Rx');
var Observable = Rx.Observable;

/** @test {bufferCount} */
Copy link
Member Author

Choose a reason for hiding this comment

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

Usually these changes just add the tag, but in other cases...

@kwonoj
Copy link
Member

kwonoj commented Feb 23, 2016

(If) #1364 is landed, does it means this PR need to be updated as well?

@staltz
Copy link
Member Author

staltz commented Feb 23, 2016

I don't know... it depends whether git is smart to identify that just the filename extension changed, and can merge the new changes.

@kwonoj
Copy link
Member

kwonoj commented Feb 23, 2016

Seems we may know after changes are landed :) let's see how it'll goes, since #1364 is not certain to be accepted at this moment.

@benlesh
Copy link
Member

benlesh commented Mar 1, 2016

This looks great.

@kwonoj
Copy link
Member

kwonoj commented Mar 1, 2016

@staltz , #1364 's checked in and seems it requires rebase. I'll check it this once it's done.

@staltz
Copy link
Member Author

staltz commented Mar 1, 2016

I'll rebase this tomorrow

@staltz
Copy link
Member Author

staltz commented Mar 2, 2016

Ok, just FYI, this is taking me some stress to rebase. ESDoc explicitly doesn't support TypeScript, so I need to compile all .ts tests to .js files, and the TS compilation isn't passing. It'll take some time to manually work this out.

@staltz
Copy link
Member Author

staltz commented Mar 2, 2016

@kwonoj we need to coordinate together the rebase I'm doing. I had to do some big changes:

  1. change spec/helpers/test-helper.ts to patch global.it = it
  2. not import it like import {it, asDiagram} from '../helpers/test-helper' in individual -spec.ts files because that would make TS compile to test_helper_1.it('should ... in -spec.js files, and that's bad for ESDoc which expects -spec.js files look like normal Mocha-style tests (with it calls inside describe calls)
  3. Change the spec compilation output from tmp to spec-js because tmp also contains tmp/docs which in turn also has javascript files (ESDoc generates some javascript) and we don't want ESDoc looking at everything inside tmp. Also, we don't want the ESDoc page to show the official path for test files being tmp/something.

Can you provide some insight why did you change 1. and 2. from how it was before? Is it solving some problem?

@staltz
Copy link
Member Author

staltz commented Mar 2, 2016

Another thing I may change is

-import {hot, cold, expectObservable, expectSubscriptions} from '../helpers/marble-testing';
 import {DoneSignature, asDiagram, lowerCaseO} from '../helpers/test-helper';
+declare const hot: any;
+declare const cold: any;
+declare const expectObservable: any;
+declare const expectSubscriptions: any;

in -spec.ts files and then

+import {assertDeepEqual, hot, cold, expectObservable, expectSubscriptions} from './marble-testing';

+global.cold = cold;
+global.hot = hot;
+global.expectObservable = expectObservable;
+global.expectSubscriptions = expectSubscriptions;

in spec/helpers/test-helper.ts

The result is that it makes the test file in ESDoc more readable:

screen shot 2016-03-02 at 17 13 19

Otherwise it looks like this:

screen shot 2016-03-02 at 17 15 43

Which is bad since we want tests (specially marble diagrams) to be additional documentation, it should be readable.

With TS2JS compilation, we lose those important whitespaces to align the ASCII marble diagrams though. I might need to try again doing a PR to TypeScript so we could add /* */ comments there to horizontally align those ASCII marble diagrams, like this:

screen shot 2016-03-02 at 17 17 23

But that has to be in a different PR, of course.

@kwonoj
Copy link
Member

kwonoj commented Mar 2, 2016

why did you change 1. and 2. from how it was before? Is it solving some problem?

: It was ergonomics changes to test cases to import helpers explicitly, intention to move forward test help to be modular without patching global variables any more. (though current still does have some) also provides type inferences to helper method too. Global patch would work, loses some conveniences though. Thinking about further plans, I'd like to have explicit export instead of global patching, but do not have great idea to solve this issue with preserving current way.

@staltz
Copy link
Member Author

staltz commented Mar 2, 2016

intention to move forward test help to be modular without patching global variables any more.

modular so that RxJS users can use the helpers too? And which helpers? I assume mainly hot, cold, etc, right? They are currently modular, packaged as helpers/marble-testing.ts. It's helpers/test-helper.ts which is doing the global patch.

I think with the current setup that we have (ESDoc only supporting JS/ES, no TS support), we have to do global patch. If we don't global patch e.g. it(), then we won't see test cases in the ESDoc test page.

When it comes to type testing our spec folder, I think the priority should to type test operators and Observables and actual RxJS stuff.

@kwonoj
Copy link
Member

kwonoj commented Mar 2, 2016

I assume mainly hot, cold, etc,

: and also was thought about test-helper to, which setup custom assertions to use hot / cold.

I think with the current setup that we have (ESDoc only supporting JS/ES, no TS support), we have to do global patch.

: Yes, I don't object that. Above comment was if I could achieve both, I'd like to have explicit export without global patching but for now global patching seems way to go for now.

@staltz
Copy link
Member Author

staltz commented Mar 2, 2016

Yeah, ESDoc not supporting TS is actually quite annoying. I've been able to get around it most of the time. Perhaps we could have looked into some other library that does straight conversion from TS -> docs, but there's probably even less tools that do that type of stuff.

Overall, I'm ok with hacking around ESDoc as we can. It's mostly useful.

@kwonoj
Copy link
Member

kwonoj commented Mar 2, 2016

Perhaps we could have looked into some other library that does straight conversion from TS -> docs

: Off-topic, but makes me curious - have we considered TypeDoc? (http://typedoc.io/) i.e unsupported feature we'd like to have, etcs?

@staltz
Copy link
Member Author

staltz commented Mar 3, 2016

Maybe we could use it, but it's unclear is it beneficial to move at this point. It doesn't seem to support showing the tests, and unclear does it support additional markdown pages organized in an outline.

@kwonoj
Copy link
Member

kwonoj commented Mar 3, 2016

it's unclear is it beneficial to move at this point.

: agreed, question's only for curiousity and not suggesting to migrate into. It could be cobsiderable only if pain of using esdoc is unbearable along with all features we'd like to have is being supported in typedoc.

@staltz staltz force-pushed the esdoc-tests branch 2 times, most recently from 5390b71 to 6bc98ef Compare March 3, 2016 12:21
@staltz
Copy link
Member Author

staltz commented Mar 3, 2016

Rebased. Also notice that I removed import {asDiagram} from 'test-helpers' because we have two different jasmine configurations, asDiagram is different for each, and we need to patch globally.

@kwonoj
Copy link
Member

kwonoj commented Mar 3, 2016

Sorry, may need another rebase :( I'll check in this once it's rebased.

Add ESDoc @test tag on each *-spec.js file in order to make it show in the ESDoc page, and also
tweak esdoc.json to show tests.
@staltz
Copy link
Member Author

staltz commented Mar 4, 2016

Rebased ✅ :)

"test": "npm run test_buildonly && npm run test_nobuild",
"test_karma": "karma start karma.conf.js",
"tests2png": "mkdirp tmp/docs/img && cp -r spec/helpers/tests2png tmp/helpers/tests2png && jasmine JASMINE_CONFIG_PATH=spec/support/tests2png.json",
"tests2png": "npm run test_buildonly && mkdirp tmp/docs/img && mkdirp spec-js/support && cp spec/support/*.json spec-js/support/ && JASMINE_CONFIG_PATH=spec/support/tests2png.json jasmine",
Copy link
Member

Choose a reason for hiding this comment

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

just note : specifying JASMINE_CONFIG_PATH prior to command jasmine makes this script windows-incompatible (was reason I changed its order to let jasmine accept it as command argument)

no blocking issue though, I'll amend this later.

Copy link
Member Author

Choose a reason for hiding this comment

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

interesting to know, sorry for breaking it

Copy link
Member

Choose a reason for hiding this comment

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

this script is rarely being used, totally ok and I can amend it later quickly. :)

@kwonoj kwonoj merged commit bd45a90 into ReactiveX:master Mar 4, 2016
@kwonoj
Copy link
Member

kwonoj commented Mar 4, 2016

Merged with bd45a90, thanks @staltz

@lock
Copy link

lock bot commented Jun 7, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants