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.

Sign up for GitHub

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

Jump to bottom

NamespacedJS tests fail if testing CommonJS code #782

Closed
janhancic opened this issue Jun 28, 2014 · 5 comments
Closed

NamespacedJS tests fail if testing CommonJS code #782

janhancic opened this issue Jun 28, 2014 · 5 comments
Labels
bug low-priority size 1
Milestone
0.12

Comments

@janhancic
Copy link
Contributor

While working on #780 I've encountered a bug. When I converted the whole library to CommonJS code ATs started failing.

After some digging I realised that it's because the tests are not CommonJS code (I have a .js-style with namespaced-js as it's contents in the tests folder) they are still using the old way of referring to classes (br.test.GwtTestRunner for instance). And that fails as there is no globalization block in the JS bundle.

That means that converting a namespaced library to common JS is extremely hard as you also have to convert all the tests before you see if you broke anything.
@janhancic
Copy link
Contributor Author

This can probably be marked as CTVP+.

@janhancic janhancic mentioned this issue Jul 1, 2014
Merged
@andy-berry-dev andy-berry-dev added this to the 0.11 milestone Jul 8, 2014
@andy-berry-dev andy-berry-dev modified the milestones: v0.12, 0.11 Jul 22, 2014
@dchambers dchambers self-assigned this Jul 22, 2014
@dchambers
Copy link
Contributor

At present, neither CommonJs or NamespacedJs style tests are properly supported. Both styles of code are properly supported however, as follows:

  • CommonJs: Wrapped in a define() block.
  • NamespacedJs: Wrapped in a define() block, with an automatically added requireAll() invocation at the top for all static dependencies used at module creation time.

If any NamespacedJs style classes are used, a globalization-block is added that contains a require() invocation for every module within the bundle. This works in tandem with the requireAll() method, which is a bit like a require() that accepts multiple require-paths, except that it also globalizes the module being required, and is tolerant of circular dependencies.

For both styles of tests to be properly supported the following changes need to be made:

  1. The globalization block should also be written when there are NamespacedJs style tests being bundled.
  2. CommonJs style tests should be placed within an IIFE so that their require() invocations aren't leaked globally.
  3. NamespacedJs style tests could be placed within an IIFE for consistency purposes, but this will potentially increase the number of breakages due to backwards compatibility.

@janhancic / @bitshifter: do you agree with this assessment?

@janhancic
Copy link
Contributor Author

👍

dchambers added a commit that referenced this issue Jul 23, 2014
@dchambers
Created a blade version of a test we already currently have at the SDK
533e3be 
level, although it turned out that #782 was actually only ocurring
because of filtering we do within the js-test-driver adaptor, and wasn't
showing within any of the spec tests we have.
@dchambers
Copy link
Contributor

The problem turned out to be some filtering code we have for js-test-driver, which removes any test source-modules since these will be separately loaded via the 'jsTestDriver.conf' file. This filtering action meant that our NamespacedJsContentPlugin came to the incorrect conclusion that there was no namespacedjs style code in use, if the only code of that style was tests.

Instead, we now wrap the BundlableNode passed to the content-plugin, so that although the test source-modules are still present, their getReader() method is wrapped so that they provide an empty response.

@dchambers dchambers removed their assignment Jul 28, 2014
@dchambers dchambers mentioned this issue Aug 6, 2014
Closed
7 tasks
@thecapdan
Copy link
Contributor

Added namespaced style acceptance test to our todo app. Passes, and namespaced style uses 'br.test.GwtTestRunner.initialize();' as mentioned to be broke by Jan, which is now in the define block

reviewed test added here: 533e3be

done

dchambers added a commit that referenced this issue Nov 25, 2014
@dchambers
Squashed 'brjs-sdk/build-resources/includes/sdk/jsdoc-toolkit-resourc…
6516eea 
…es/jsdoc-toolkit/' changes from 5b8b723..7212426

7212426 Renamed the BladeRunnerJS stylesheet to 'brjs.css' so it's clear what it is.
60f51d0 Change filter-input-box button offsets so it works with the BladeRunnerJS stylesheet.
c784793 Add padding to div#main instead of requiring the <h1/> element within it to set margin-left and margin-top.
aef86c5 Show modules at the end of the index since they are less important than the things that the modules contain.
a0154e0 Work-around for <jsdoc/jsdoc#763> that allows our NamespacedJs style classes to have their methods and static constants to continue to be displayed without breaking any of the existing spec tests -- static methods will not currently display.
de3224e Updated so the dashboard navigation is once again displayed by default, but where the docs uploaded to <http://apidocs.bladerunnerjs.org/> don't have the dashboard navigation.
1ebd42d Added the BladeRunnerJS styled navigation to the jsdocs. Conflicts: 	templates/default/tmpl/layout.tmpl
cad625a Added the 'hosted-docs.css' stylesheet that we are now linking too.
50d8748 Update default templates to help integrate with the BladeRunnerJS dashboard.
7525600 Merge branch 'index-filtering-combo-box2' into brjs-fresh
3dc491a Switched from 'better-placeholder-polyfill' to 'Placeholder.js' because the 'Placeholder.js' has zero dependencies and works more reliably.
d991158 Added a shim so IE9 and below can see the filter box placeholder text, plus moved all shims into the same area.
e2c6ac4 Final nudge to make the index filter styling continue work now it no longer has access to the BRJS stylesheets it was originally developed alongside.
3d13749 Include the script necesarry to use the index filter.
084138e Work around the bug that occurs in IE10 and below (pressing enter in a text box causes the first button in the same form to be clicked) by explicitly wrapping in a <form/> tag in case other buttons are added to the page (we do this in the BladeRunnerJS template), and ensuring that the other button in the same form is not considered to be a submit button.
318032d Clean up how we are doing the browser specific fix-ups so they are only applied on the relevant browsers. Conflicts: 	templates/default/tmpl/layout.tmpl
196ceaf Use a PNG instead of an SVG for the 'x' button so that it can be viewed in IE8. Conflicts: 	templates/default/static/styles/styles.css
8a3a47b re-added html5shiv as the style stopped working on IE8 after I removed it.
8dbb24f Make the index filter work in IE8 -- this has also fixed a script error that was previously occurring in IE8 anyway. Conflicts: 	templates/default/tmpl/layout.tmpl
e7f7961 Added index filtering support to jsdoc so that large indexes can be filtered by package. Conflicts: 	templates/default/static/scripts/functions.js 	templates/default/static/styles/styles.css
5eaabcf Attempt to fix remaining linting errors -- may not work as I'm unable to run the build locally on my own machine to verify.
0137865 Attempt to fix remaining linting errors -- may not work as I'm unable to run the build locally on my own machine to verify.
7d80410 Removed trailing whitespace from 'publish.js'.
773b785 Ensure the title for classes defined within modules shows the thing being documented as being a class rather than a module, as it currently does for interfaces.
9fc527d Remove the 'module:' prefix from index links to classes and interfaces defined within modules.
21e3e31 Removed duplicated code from the buildNav() function, where the version of the code used is one where items are only written if they haven't already been encountered, and a heading is only written when there are items to appear beneath that heading. Conflicts: 	templates/default/publish.js
1a975c0 remove duplicate `Requires` section in modules (#790)
42f388f render a doclet's `overrides` property (#792)
afbc4fa close `li` tag
b22a8a2 add `overrides` property to doclets that override other symbols (#792)
93cfe29 delint
7807ac0 rename "Index" to "Home" (#750)
36065d2 add `exception`/`throws` to default tag list for Markdown plugin (#736)
8f1d892 ensure that filenames do not start with an underscore (#758)
df1f4bd treat `foo="bar"` as a non-optional version of `[foo="bar"]` (#791)
d17ecfd remove cruft from Tag objects
bfddc53 Merge pull request #802 from cdparks/fallback-to-regex
768a61c Falling back to regex solution for default value when value is malformed
5cff804 detab
7761292 use nicer typography in the default template (#780)
1f71ae0 fix bad test case
06acc66 optionally scan tutorials directory recursively (#712)
5399745 allow default values for parameters to contain brackets (#640)
2224bee do not try to normalize empty path strings
fc2b4ad fix Windows path issues and static-file copying (#785)
d2e5dd8 fix gulpfile on Windows when the working directory contains spaces
a2e119a update ESLint dependency; enable new rules; delint
b28f334 delint
e60fc3c prevent duplication when two parent classes have instance members with identical names (#613)
996c283 handle symbol names with leading/trailing whitespace (#549)
5883922 strip module namespace from `module` and `exports` tag values (#786)
5512af9 update package schema, and add tests to validate package objects against the schema (#788)
53d2ed7 prevent cruft in Package objects (#787)
7471685 show the inheritance chain for modules that export a single constructor (#594)
e5a3479 fix an accidental method-signature change
ce79d0d bump revision
c81ace8 extract more information from package.json files; add tests (#710)
0ec1386 use the non-deprecated property for license info
c45fdaa allow any file to be used as a package or README file (#708)
25ffab0 prevent a crash when a package file specifies a name, but not a version
8d36936 remove dead code
9e699ff omit non-enumerable properties from cloned objects (#784)
c832f24 remove unused requires
d28d15e remove unused code
c510b33 fix performance regression caused by 2eb3c46 (#784)
2b34c51 clarify logic for resolving property parents
79a3be1 hoist requires
8003ce1 cleanup
2eb3c46 handle enums that are part of a chain of assignments (#702)
e69001d correct a comment
8f231c1 enable a test case
68ceb33 stop adding a `scope` property to module doclets that include an `exports` tag (#782)
bb8a662 use enum value for global scope
f77984d fix name resolution when the `exports` tag is used on a pointer to the module's `exports` object (#404)
1774569 delint
612b96c remove obsolete note from README
8238bc3 update Catharsis (fixes #652 and #767)
c083ab7 remove unused test; cleanup
413c7eb whitespace
00b9dbb don't write empty class descriptions to template output (#741)
3b865dd Merge remote-tracking branch 'brjs/fix-classdesc-in-modules'
4eb86a1 handle object literals whose property names must be escaped in a regexp (#775)
765abd9 cleanup
df7a1e0 do not add an exception's description and type to the parent doclet; avoid circular refs when cloning an object (#772)
b594458 update committed dependencies
908e5e6 do not unescape entities in Markdown tutorials (#743)
ebb6b22 cleanup
010cd73 bump revision
7399895 fix `exports` tag when the module object is passed to an AMD function (#642)
71de4ac cleanup
d527f3e prevent crash when a `returns` tag does not have a value (#751)
e093716 handle sparse arrays correctly in nodeToString (#749)
6328336 cleanup
81d74b9 remove scope property from module doclets (#742)
55f40e1 cleanup
REVERT: 5b8b723 Merge branch 'index-filtering-combo-box' into brjs
REVERT: 6afb71e Switched from 'better-placeholder-polyfill' to 'Placeholder.js' because the 'Placeholder.js' has zero dependencies and works more reliably.
REVERT: 686837b Merge branch 'index-filtering-combo-box' into brjs
REVERT: 3bf2173 Added a shim so IE9 and below can see the filter box placeholder text, plus moved all shims into the same area.
REVERT: 1f59b28 Final nudge to make the index filter styling continue work now it no longer has access to the BRJS stylesheets it was originally developed alongside.
REVERT: d9017e8 Include the script necesarry to use the index filter.
REVERT: 63c049b fixed remaining linting errors.
REVERT: 0270f97 fix all linting errors except 'no-use-before-define', since these might cause merge conflicts if mixed in with the other error fixes.
REVERT: 9ef4088 Work around the bug that occurs in IE10 and below (pressing enter in a text box causes the first button in the same form to be clicked) by explicitly wrapping in a <form/> tag in case other buttons are added to the page (we do this in the BladeRunnerJS template), and ensuring that the other button in the same form is not considered to be a submit button.
REVERT: 46deae9 Clean up how we are doing the browser specific fix-ups so they are only applied on the relevant browsers. Conflicts: 	templates/default/tmpl/layout.tmpl
REVERT: feb7891 Use a PNG instead of an SVG for the 'x' button so that it can be viewed in IE8. Conflicts: 	templates/default/static/styles/styles.css
REVERT: e366264 re-added html5shiv as the style stopped working on IE8 after I removed it.
REVERT: 48ca8f6 Make the index filter work in IE8 -- this has also fixed a script error that was previously occurring in IE8 anyway. Conflicts: 	templates/default/tmpl/layout.tmpl
REVERT: efc5aa6 Added index filtering support to jsdoc so that large indexes can be filtered by package. Conflicts: 	templates/default/static/scripts/functions.js 	templates/default/static/styles/styles.css
REVERT: 9a7fccd Renamed 'Index' to 'Home' since 'index.html' does not actually contain an index -- the index is actually available on the right of every page.
REVERT: 6547252 Add padding to div#main instead of requiring the <h1/> element within it to set margin-left and margin-top.
REVERT: 2fa1123 Show modules at the end of the index since they are less important than the things that the modules contain.
REVERT: 39b5385 Ensure the title for classes defined within modules shows the thing being documented as being a class rather than a module, as it currently does for interfaces.
REVERT: 3c5aa98 Remove the 'module:' prefix from index links to classes and interfaces defined within modules.
REVERT: 45f67fd Removed duplicated code from the buildNav() function, where the version of the code used is one where items are only written if they haven't already been encountered, and a heading is only written when there are items to appear beneath that heading.
REVERT: 59a2614 Accidentally missed from previous check-in.
REVERT: bb16a44 Updated the license to reflect the inclusion of the Open Sans typeface, plus included EOT and SVG versions of the font so that older version of IE and Android browser also get to see the page rendered correctly.
REVERT: c7359fe Rebased the template typography off of the Mozilla Developer Network site so generated API docs have a more contemporary and familiar style.
REVERT: 7dfb471 Updated the template so it now display the @classdesc for classes defined within the modules.
REVERT: 76a0306 Display the class description even when the class is defined within a module.
REVERT: 277bbd7 Added failing test that should work (it works within the template) and which otherwise explains how the class information is available for use within module documentation pages.
REVERT: 0d4df18 Added missing coverage around modules that contain a default CommonJs class being exported within the module -- this has actually shown that the doclets produced are probably correct, and that the bug is likely to be within the template.
REVERT: 09d199b Work-around for <jsdoc/jsdoc#763> that allows our NamespacedJs style classes to have their methods and static constants to continue to be displayed without breaking any of the existing spec tests -- static methods will not currently display.
REVERT: f8d61e9 Merging changes from 'index-filtering-support' branch
REVERT: 765b3ec fixed remaining linting errors.
REVERT: 01530b2 fix all linting errors except 'no-use-before-define', since these might cause merge conflicts if mixed in with the other error fixes.
REVERT: ef21fed Work around the bug that occurs in IE10 and below (pressing enter in a text box causes the first button in the same form to be clicked) by explicitly wrapping in a <form/> tag in case other buttons are added to the page (we do this in the BladeRunnerJS template), and ensuring that the other button in the same form is not considered to be a submit button.
REVERT: b236436 Clean up how we are doing the browser specific fix-ups so they are only applied on the relevant browsers.
REVERT: ea3dd02 Prevent IE8 from trying to submit when the enter key is pressed.
REVERT: f9d3ed5 Use a PNG instead of an SVG for the 'x' button so that it can be viewed in IE8.
REVERT: 923d4e8 Merge commit 'ba93ca08813d89e79927bf514208c8a994c7984d' into jsdoc-index-filtering-support
REVERT: 898e2b9 Merge branch 'jsdoc-improvements' into jsdoc-index-filtering-support
REVERT: 06ad31c re-added html5shiv as the style stopped working on IE8 after I removed it.
REVERT: cda906e re-added html5shiv as the style stopped working on IE8 after I removed it.
REVERT: c7c968d Merge branch 'rebase-typography-off-of-mdn' into brjs
REVERT: 312d4e5 Don't allow the title to overflow when using a lower resolution display.
REVERT: 96b84c0 Make the index filter work in IE8 -- this has also fixed a script error that was previously occurring in IE8 anyway.
REVERT: 6e10340 Added index filtering support to jsdoc so that large indexes can be filtered by package.
REVERT: 35ccedc Merge branch 'rename-index-to-home' into brjs
REVERT: 745f6bf Renamed 'Index' to 'Home' since 'index.html' does not actually contain an index -- the index is actually available on the right of every page.
REVERT: a7fd611 Merge branch 'rebase-typography-off-of-mdn' into brjs
REVERT: 305dbb6 Add padding to div#main instead of requiring the <h1/> element within it to set margin-left and margin-top.
REVERT: 03039f3 Updated so the dashboard navigation is once again displayed by default, but where the docs uploaded to <http://apidocs.bladerunnerjs.org/> don't have the dashboard navigation.
REVERT: 4a7040a Added the BladeRunnerJS styled navigation to the jsdocs.
REVERT: 1640ef1 Added the 'hosted-docs.css' stylesheet that we are now linking too.
REVERT: 8312b3d Update default templates to help integrate with the BladeRunnerJS dashboard.
REVERT: a74a4eb Merge branch 'show-modules-last-in-index' into brjs
REVERT: 8191515 Show modules at the end of the index since they are less important than the things that the modules contain.
REVERT: 8cf8438 Merge branch 'index-usability-improvements' into brjs
REVERT: 05d89c2 Ensure the title for classes defined within modules shows the thing being documented as being a class rather than a module, as it currently does for interfaces.
REVERT: 82e8f3c Remove the 'module:' prefix from index links to classes and interfaces defined within modules.
REVERT: 969eb82 Removed duplicated code from the buildNav() function, where the version of the code used is one where items are only written if they haven't already been encountered, and a heading is only written when there are items to appear beneath that heading.
REVERT: 8f92565 Accidentally missed from previous check-in.
REVERT: e98b060 Updated the license to reflect the inclusion of the Open Sans typeface, plus included EOT and SVG versions of the font so that older version of IE and Android browser also get to see the page rendered correctly.
REVERT: 7e1a581 Rebased the template typography off of the Mozilla Developer Network site so generated API docs have a more contemporary and familiar style.

git-subtree-dir: brjs-sdk/build-resources/includes/sdk/jsdoc-toolkit-resources/jsdoc-toolkit
git-subtree-split: 72124262d2eafbbd2e7b7d60059f9a87372c6e72
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug low-priority size 1
Projects
None yet
Milestone
0.12
Development

No branches or pull requests
4 participants
@dchambers @janhancic @andy-berry-dev @thecapdan