Add tests for existing functionality #9

leoj3n · 2017-04-28T18:58:45Z

Changes some of the code in demo_frame.js, most notably show now sets display to '' instead of 'block'. Shouldn't cause any issues on any of the sites, but that is more correct.

The tests should be pretty comprehensive, but could add a test for a more complex example.
See test/basics/demo-complex.html for an example of a complex demo tag usage.
Update: Complex demo tag usage example test has been added.

❯ npm test

> bit-docs-tag-demo@0.3.0 test /Users/leoj/bitovi/,+/bd/1/bit-docs-tag-demo
> mocha test.js --reporter spec



  bit-docs-tag-demo
    temp directory
      ✓ is generated (13981ms)
    demo widget
      with ids
        ✓ exists on page
        tabs and contents
          ✓ has three
          ✓ only one active
          ✓ defaults to demo
        clicking HTML tab
          ✓ changes tab and content
        Demo
          iframe (demo-with-ids.html)
            ✓ has correct url and parent
            ✓ has correct content
        HTML
          data-for=html
            ✓ has correct content
        JS
          data-for=js
            ✓ has correct content
      without ids
        ✓ exists on page
        tabs and contents
          ✓ has three
          ✓ only one active
          ✓ defaults to demo
        clicking HTML tab
          ✓ changes tab and content
        Demo
          iframe (demo-without-ids.html)
            ✓ has correct url and parent
            ✓ has correct content
        HTML
          data-for=html
            ✓ has correct content
        JS
          data-for=js
            ✓ has correct content
      without js
        ✓ exists on page
        tabs and contents
          ✓ has three
          ✓ only one active
          ✓ defaults to demo
        clicking HTML tab
          ✓ changes tab and content
        Demo
          iframe (demo-without-js.html)
            ✓ has correct url and parent
            ✓ has correct content
        HTML
          data-for=html
            ✓ has correct content
        JS
          ✓ tab is hidden
          data-for=js
            ✓ has correct content
      complex
        ✓ exists on page
        tabs and contents
          ✓ has three
          ✓ only one active
          ✓ defaults to demo
        clicking HTML tab
          ✓ changes tab and content
        Demo
          iframe (demo-complex.html)
            ✓ has correct url and parent
            ✓ has correct content
        HTML
          data-for=html
            ✓ has correct content
        JS
          data-for=js
            ✓ has correct content
      multiple instances
        ✓ exist on page
        clicking all HTML tabs
          ✓ changes all tabs and contents


  40 passing (18s)

leoj3n · 2017-04-29T05:54:43Z

🥇 Reviewers:

I'd like feedback and to get this merged sooner than later, as it may influence tests I write elsewhere.
Thanks!

chasenlehara

I left a few suggestions 🙂

chasenlehara · 2017-04-29T18:08:06Z

demo_frame.js

@@ -86,11 +88,11 @@ module.exports = function(node){
 					}
 				}
 			}
-			return source.trim();
+			return (source ? source.trim() : false);


I’d recommend just returning a string, not a string or boolean.

Good point.

chasenlehara · 2017-04-29T18:29:27Z

test/basics/demo-complex.html

+
+<script src="../../node_modules/steal/steal.js" main="@empty" id='demo-source'>
+import "can-view-autorender";
+import "can-stache-bindings";


It seems excessive to bring in so many dependencies for a test. Is this one needed? Are there any others that could be removed?

I was wondering the same thing, but I don't know much about CanJS. Can you recommend a better "complex" test to do? I like this one because it takes a second (literally) to load.

can-stache-bindings is for stuff like {prop}="value"

What specifically did you want to test? To me, it looks like “complex” means loading modules with StealJS, so I would probably simplify the file to something like this:

<script src="../../node_modules/steal/steal.js" main="@empty" id='demo-source'> import stache from "can-stache"; var renderer = stache("<h1>Hello {{subject}}</h1>"); var fragment = renderer({subject: "World"}) document.body.appendChild(fragment) </script>

There was a (click) handler that was causing errors under zombie that I removed, so that explains where the can-stache-bindings came from. The error had to do with cloneNode, but don't think that's something we would want to resolve just to support zombie. I took this from the canjs/canjs repo, so it's a real example, which I thought would be important to include next to the overly simple examples. The deps aren't that big of a burden since they're small and quick to install, and are dev only, but I can certainly simplify it if we're not losing anything beneficial by doing so.

I made this change almost exactly as you have it here. Thanks for the snippet.

chasenlehara · 2017-04-29T18:33:31Z

test.js

+	assert = require('assert'),
+	Browser = require('zombie'),
+	express = require('express'),
+	generate = require('bit-docs-generate-html/generate');


You don’t have to change this[1], but I’d recommend keeping variable declarations on separate lines. In this case, it’d be easier to scan through the list if they were formatted the same way and alphabetical.

[1] As far as I can tell, this isn’t covered in jQuery’s style guide… now I see why someone (@andrejewski?) was advocating for stricter guidelines.

Well, the use of tabs is something I picked from other parts of the bit-docs codebase, and it is my understanding that it is @justinbmeyer's preference. I personally prefer spaces, but technically these do line up properly with your editor configured properly:

The spacing on GitHub's website could be resolved by adding .editorconfig.

I thought tabs were desired, and that's why I used them here. I'm fine with changing to using var on each declaration, replacing tabs with spaces, or adding .editorconfig.

At the end of the day, these tests do their job, and that's most important! I'll leave this for now, as eventually I imagine we'll run everything through a code formatter anyways.

I didn’t say anything about using spaces instead of tabs. 🙂

For me, the odd spacing would stand out as a bigger issue when it comes to "easier to scan through the list if they were formatted the same way", and assumed that's what you meant.

The fact that I'm using tabs instead of spaces is a testament to the fact that I'm not having personal preference about formatting. I will change it however you like, but I was mostly concerned about getting your feedback on the functionality and test coverage!

Change made.

Still need to automate npm install, and make work with zombie.

leoj3n · 2017-05-10T15:51:45Z

With current commits, assuming you manually run npm install inside of test/demos/demo-complex, the demo will work fine in a normal browser, but zombie seems to have the following issue...

(when running npm test):

{ Error: Error loading "package.json!npm" at <unknown>
Error loading "npm" at http://example.com/node_modules/steal/ext/npm.js
Not Found: http://example.com/node_modules/steal/ext/npm.js
    at error (http://example.com/demos/demo-complex/node_modules/steal/steal.js:3069:19)
    at XMLHttpRequest.xhr.onreadystatechange (http://example.com/demos/demo-complex/node_modules/steal/steal.js:3079:13)
    at XMLHttpRequest.callback.(anonymous function) (/Users/leoj/bitovi/,+/bd/1/bit-docs-tag-demo/node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:230:32)
    at invokeEventListeners (/Users/leoj/bitovi/,+/bd/1/bit-docs-tag-demo/node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:195:25)
    at invokeInlineListeners (/Users/leoj/bitovi/,+/bd/1/bit-docs-tag-demo/node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:163:7)
    at EventTargetImpl._dispatch (/Users/leoj/bitovi/,+/bd/1/bit-docs-tag-demo/node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:122:7)
    at EventTargetImpl.dispatchEvent (/Users/leoj/bitovi/,+/bd/1/bit-docs-tag-demo/node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:87:17)
    at XMLHttpRequest.dispatchEvent (/Users/leoj/bitovi/,+/bd/1/bit-docs-tag-demo/node_modules/jsdom/lib/jsdom/living/generated/EventTarget.js:60:35)
    at XMLHttpRequest.DOM.EventTarget.dispatchEvent (/Users/leoj/bitovi/,+/bd/1/bit-docs-tag-demo/node_modules/zombie/lib/dom/jsdom_patches.js:182:31)
    at XMLHttpRequest._fire (/Users/leoj/bitovi/,+/bd/1/bit-docs-tag-demo/node_modules/zombie/lib/xhr.js:248:12) statusCode: 404 } 'Error: Error loading "package.json!npm" at <unknown>\nError loading "npm" at http://example.com/node_modules/steal/ext/npm.js\nNot Found: http://example.com/node_modules/steal/ext/npm.js\n    at error (http://example.com/demos/demo-complex/node_modules/steal/steal.js:3069:19)\n    at XMLHttpRequest.xhr.onreadystatechange (http://example.com/demos/demo-complex/node_modules/steal/steal.js:3079:13)\n    at XMLHttpRequest.callback.(anonymous function) (/Users/leoj/bitovi/,+/bd/1/bit-docs-tag-demo/node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:230:32)\n    at invokeEventListeners (/Users/leoj/bitovi/,+/bd/1/bit-docs-tag-demo/node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:195:25)\n    at invokeInlineListeners (/Users/leoj/bitovi/,+/bd/1/bit-docs-tag-demo/node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:163:7)\n    at EventTargetImpl._dispatch (/Users/leoj/bitovi/,+/bd/1/bit-docs-tag-demo/node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:122:7)\n    at EventTargetImpl.dispatchEvent (/Users/leoj/bitovi/,+/bd/1/bit-docs-tag-demo/node_modules/jsdom/lib/jsdom/living/events/EventTarget-impl.js:87:17)\n    at XMLHttpRequest.dispatchEvent (/Users/leoj/bitovi/,+/bd/1/bit-docs-tag-demo/node_modules/jsdom/lib/jsdom/living/generated/EventTarget.js:60:35)\n    at XMLHttpRequest.DOM.EventTarget.dispatchEvent (/Users/leoj/bitovi/,+/bd/1/bit-docs-tag-demo/node_modules/zombie/lib/dom/jsdom_patches.js:182:31)\n    at XMLHttpRequest._fire (/Users/leoj/bitovi/,+/bd/1/bit-docs-tag-demo/node_modules/zombie/lib/xhr.js:248:12)'

Perhaps something in steals npm ext is not compatible with jsdom?:

https://github.com/stealjs/steal/blob/master/ext/npm-load.js

It would be nice to have encapsulation of the demo deps, as mentioned in #11.

chasenlehara · 2017-05-11T16:33:00Z

@leoj3n Is that blocking this from being merged? If not, will you merge this?

leoj3n · 2017-05-11T18:32:27Z

The latest commits are blocking; I should have made them a separate branch/PR (though, I didn't expect zombie to error after some simple reorganization).

I'll back out the commits for isolating demo deps, so that this PR will be merge-able again, and break out that idea into it's own PR.

Also simplifies the "complex" demo and test. Additionally, moves location of "temp" into test/

leoj3n · 2017-05-11T19:34:03Z

@chasenlehara Should be merge-able again now.

chasenlehara · 2017-05-11T23:56:37Z

Ok go for it

leoj3n added 12 commits April 20, 2017 15:21

mocha and qunit tests working

0de840e

zombie, connect, http, server-static

4a9fa09

use express, comment out failing zombie asserts

bfe9009

Add testee (WIP)

8126347

Real tests using zombie

c700d5d

zombie uses jsdom which does not support dataset

05b61a7

Make demo_frame.js smarter

78c0fe3

More comprehensive tests

c359ecf

Verify HTML and JS content in tests

476ff1e

31 tests passing

db214ef

Remove unused stuff

18a20c0

Add complex demo test

00d53ad

leoj3n mentioned this pull request Apr 29, 2017

Demos now grow up to 600px to fit content #5

Closed

leoj3n requested review from justinbmeyer and chasenlehara April 29, 2017 05:53

chasenlehara approved these changes Apr 29, 2017

View reviewed changes

leoj3n added 4 commits May 9, 2017 15:15

Remove can-stache-bindings dependency

ffbd91d

Return empty string instead false

5f017c6

Separate var statements to one line each

c1224ca

Sort var statements alphabetically

6fe23a5

leoj3n self-assigned this May 9, 2017

leoj3n added the enhancement label May 9, 2017

leoj3n added 3 commits May 9, 2017 17:58

Make things more readable

e12d5ad

Move things around for organization

2a3450d

Encapsulate complex demo dependencies

def26e7

Still need to automate npm install, and make work with zombie.

Back out encapsulation of demo deps

0370def

Also simplifies the "complex" demo and test. Additionally, moves location of "temp" into test/

leoj3n merged commit 7e45843 into master May 12, 2017

chasenlehara deleted the mocha-qunit-tests-working branch December 29, 2017 00:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests for existing functionality #9

Add tests for existing functionality #9

leoj3n commented Apr 28, 2017 •

edited

Loading

leoj3n commented Apr 29, 2017 •

edited

Loading

chasenlehara left a comment

chasenlehara Apr 29, 2017

leoj3n Apr 30, 2017

chasenlehara Apr 29, 2017

leoj3n Apr 30, 2017

chasenlehara Apr 30, 2017

leoj3n Apr 30, 2017 •

edited

Loading

leoj3n May 11, 2017

chasenlehara Apr 29, 2017

leoj3n Apr 30, 2017 •

edited

Loading

chasenlehara Apr 30, 2017

leoj3n Apr 30, 2017

leoj3n May 11, 2017

leoj3n commented May 10, 2017 •

edited

Loading

chasenlehara commented May 11, 2017

leoj3n commented May 11, 2017

leoj3n commented May 11, 2017

chasenlehara commented May 11, 2017

Add tests for existing functionality #9

Add tests for existing functionality #9

Conversation

leoj3n commented Apr 28, 2017 • edited Loading

leoj3n commented Apr 29, 2017 • edited Loading

chasenlehara 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

leoj3n Apr 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leoj3n Apr 30, 2017 • edited Loading

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

leoj3n commented May 10, 2017 • edited Loading

chasenlehara commented May 11, 2017

leoj3n commented May 11, 2017

leoj3n commented May 11, 2017

chasenlehara commented May 11, 2017

leoj3n commented Apr 28, 2017 •

edited

Loading

leoj3n commented Apr 29, 2017 •

edited

Loading

leoj3n Apr 30, 2017 •

edited

Loading

leoj3n Apr 30, 2017 •

edited

Loading

leoj3n commented May 10, 2017 •

edited

Loading