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

Spec test update for define without dependencies, correction for anon_relative #17

Merged
merged 5 commits into from
Mar 28, 2013
Merged

Spec test update for define without dependencies, correction for anon_relative #17

merged 5 commits into from
Mar 28, 2013

Conversation

thecodedrift
Copy link
Contributor

This creates 2 new tests:

  • basic_empty_deps - test define() when there is dependencies defined as undefined and when dependencies are defined as []
  • config_paths_relative - test that when relative paths are used, they are relative to the Module ID and not to the module's final-resolved URL.

This changes the following Library Compliance:

  • RequireJS - commented basic until next rev, which resolves basic_empty_defs
  • LSJS - commented basic until next rev, which resolves basic_empty_defs

No current test suites are impacted by config_paths_relative.

Referenced Issues:
This is a rollup for #13 #14 #15 #16

The original anon_relative was using logic required by config_path
in order to properly test the relative path information. To remedy,
the anon_relative has the path information removed, and a new test
config_path_relative was created as part of the configPaths suite.
This test ensures that relative modules remain relative to their
module ID and not to their URL, even in the event of a path remap.

Github ID: #13
Issue #14 references a situation where an empty dependency array
denoted by [] should return no dependencies (as opposed to the
normal require/exports/module combination). A define() call that
does not contain any dependencies should receive the default
require/module/exports combination.
Additionally, basic tests were commented for the libraries out of
spec to ensure we don't miss when we break other things.
thecodedrift added a commit that referenced this pull request Mar 28, 2013
Spec test update for define without dependencies, correction for anon_relative

Fixes #13 #14 #15 #16
@thecodedrift thecodedrift merged commit 6342d5d into amdjs:master Mar 28, 2013
@rbackhouse
Copy link
Contributor

So I have been looking at the new test more closely and it seems that it will be a problem with loaders that do factory-on-require processing. The test is defining modules with the assumption that the factory call will be triggered by the define call itself. This won't work for loaders that follow the factory-on-require pattern (See https://groups.google.com/forum/?fromgroups=#!searchin/amd-implement/factory-on-define/amd-implement/ot46tE-N6L0/gUzoJhpsukIJ for more details).

lsjs actually follows factory-on-require and the fix i delivered really isn't the correct solution for it.

I think the test should look more like this where "require" is used to trigger the factory call.

go(["_reporter", "require"], function(amdJS, require) {

  function emptyDeps(then) {
    define('emptyDeps', [], function() {
      amdJS.assert(arguments.length === 0, 'basic_empty_deps: [] should be treated as no dependencies instead of the default require, exports, module');
    });
    then();
  }

  function noDeps(then) {
    define('noDeps', function(require, exports, module) {
      amdJS.assert(typeof(require) === 'function', 'basic_empty_deps: no dependencies case uses require in first slot. Is a function');
      amdJS.assert(typeof(exports) === 'object', 'basic_empty_deps: no dependencies case uses exports in second slot. Is an object.');
      amdJS.assert(typeof(module) === 'object', 'basic_empty_deps: no dependencies case uses module in third slot. Is an object.');
    });
    then();
  }

  // this nesting structure ensures that the AMD define will resolve
  // before we call the next by after the tests are ran in each use
  // case. We use named define calls to ensure there are not module
  // conflicts or mismatches that can occur using anonymous modules.
  emptyDeps(function () {
    require(['emptyDeps'], function() {
        window.setTimeout(function () {
          noDeps(function () {
            require(['noDeps'], function() {
                window.setTimeout(function () {
                  amdJS.print('DONE', 'done');
                });
            });
          });
        });
    });
  });

});

@thecodedrift
Copy link
Contributor Author

I think that's pretty valid. If someone has factory-on-require, they would unfairly fail these tests, so we should ensure their factories are given the opportunity to run. I'll put together a PR that references this change

thecodedrift added a commit that referenced this pull request Mar 29, 2013
Update on #17, Splits tests, uses internal require
@thecodedrift
Copy link
Contributor Author

Done and done. Thanks @rbackhouse for the assist!

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.

2 participants