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

Nested modules #859

Closed
wants to merge 3 commits into
from

Conversation

4 participants
@leobalter
Member

leobalter commented Sep 10, 2015

Replaces/Closes #800
Closes #543

This PR closes #800 with extra additions to the PR to assure a good landing of nested modules.

We might want to observe the issues listed at #858 and add extra commits

cc @gibson042

QUnit.test( "modules with nested functions does not spread beyond", function( assert ) {
assert.equal( assert.test.module.name, "pre-nested modules" );
});

This comment has been minimized.

@leobalter

leobalter Sep 10, 2015

Member

@jzaefferer @gibson042 I need opinions on this test.

@leobalter

leobalter Sep 10, 2015

Member

@jzaefferer @gibson042 I need opinions on this test.

This comment has been minimized.

@gibson042

gibson042 Sep 11, 2015

Member

Seems ok to me. The interaction itself sucks, but the test captures it well.

@gibson042

gibson042 Sep 11, 2015

Member

Seems ok to me. The interaction itself sucks, but the test captures it well.

@leobalter

This comment has been minimized.

Show comment
Hide comment
@leobalter

leobalter Sep 29, 2015

Member

@gibson042 I made two commits to include arguments on the contained modules callback. I appreciate if you review and criticize.

cc @jzaefferer

Member

leobalter commented Sep 29, 2015

@gibson042 I made two commits to include arguments on the contained modules callback. I appreciate if you review and criticize.

cc @jzaefferer

beforeEach( function( assert ) {
assert.ok( true, "beforeEach called" );
} );

This comment has been minimized.

@leobalter

leobalter Sep 29, 2015

Member

This is even better on ES6:

QUnit.module( "foo", ( test, { beforeEach, afterEach } ) => {
  beforeEach: () => {
    ...
  };

  ...
});

Also, a funny fact:

const describe = QUnit.module;

describe( "wat", it => {
  it( "works", ( { ok } ) => {
    ok( true, "omg" );
  });
});
@leobalter

leobalter Sep 29, 2015

Member

This is even better on ES6:

QUnit.module( "foo", ( test, { beforeEach, afterEach } ) => {
  beforeEach: () => {
    ...
  };

  ...
});

Also, a funny fact:

const describe = QUnit.module;

describe( "wat", it => {
  it( "works", ( { ok } ) => {
    ok( true, "omg" );
  });
});

leobalter added a commit to leobalter/api.qunitjs.com that referenced this pull request Sep 29, 2015

Show outdated Hide outdated src/core.js_
@@ -0,0 +1,312 @@
QUnit.urlParams = urlParams;

This comment has been minimized.

@gibson042

gibson042 Sep 30, 2015

Member

I'm assuming these .js_ files are accidental additions.

@gibson042

gibson042 Sep 30, 2015

Member

I'm assuming these .js_ files are accidental additions.

This comment has been minimized.

@leobalter

leobalter Sep 30, 2015

Member

caught in a git add .

@leobalter

leobalter Sep 30, 2015

Member

caught in a git add .

This comment has been minimized.

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Sep 30, 2015

Member

I made two commits to include arguments on the contained modules callback. I appreciate if you review and criticize.

I'm not sure about these particular parameters. As I've noted elsewhere, I'd like module and test callback signatures to look as similar as possible. But further than that, I'd like module callbacks to be straightforward. It seems to me that data passed in to them—including as this—should be entirely sufficient to define submodules/tests/etc., but hopefully not exceed that.

In my opinion, it's better to pass nothing at all now than it is to guess wrong about the signature. Since this iteration still requires access to the global QUnit for some operations (e.g., submodules), I'd actually prefer an argument-free executeNow() for this PR, saving context/arguments for later as discussed. For example, a possible future that would be precluded by passing test now:

QUnit.module("outer", function( outerModule ) {
    // environment (prototype for test/submodule context)
    this.outer = true;
    this.outerBefore = 0;

    // test before/after
    outerModule.test.beforeEach(function() {
        this.outerBefore++;
    });

    // tests
    outerModule.test("foo", function( assert ) {
        assert.equal(this.outerBefore, 1, "beforeEach updated environment");
    });
    outerModule.test("bar", function( assert ) {
        assert.equal(this.outerBefore, 1, "sibling tests don't share environments");
    });

    // submodules
    outerModule.module("inner", function( innerModule ) {
        // environment (inherits from parent)
        this.inner = true;

        function assertEnvironment( assert, before ) {
            assert.ok(this.outer, "outer module initialized environment");
            assert.ok("outerBefore" in this, "outer module beforeEach updated environment");
            assert.equal(this.outerBefore, 1, "submodules don't share environments");
            assert.ok(this.inner, "inner module initialized environment");
            if ( before ) {
                return;
            }
            assert.ok(this.innerBefore, "inner module beforeEach updated environment");
            assert.ok(this.tested, "test updated environment");
        }

        // test before/after
        innerModule.test.beforeEach(function( assert ) {
            this.innerBefore = true;
            assertEnvironment.call( this, assert, true );
        });
        innerModule.test.afterEach(function( assert ) {
            assertEnvironment.call( this, assert );
        });

        // tests
        innerModule.test("bar", function( assert ) {
            this.tested = true;
            assertEnvironment.call( this, assert );
        });
    });
});
Member

gibson042 commented Sep 30, 2015

I made two commits to include arguments on the contained modules callback. I appreciate if you review and criticize.

I'm not sure about these particular parameters. As I've noted elsewhere, I'd like module and test callback signatures to look as similar as possible. But further than that, I'd like module callbacks to be straightforward. It seems to me that data passed in to them—including as this—should be entirely sufficient to define submodules/tests/etc., but hopefully not exceed that.

In my opinion, it's better to pass nothing at all now than it is to guess wrong about the signature. Since this iteration still requires access to the global QUnit for some operations (e.g., submodules), I'd actually prefer an argument-free executeNow() for this PR, saving context/arguments for later as discussed. For example, a possible future that would be precluded by passing test now:

QUnit.module("outer", function( outerModule ) {
    // environment (prototype for test/submodule context)
    this.outer = true;
    this.outerBefore = 0;

    // test before/after
    outerModule.test.beforeEach(function() {
        this.outerBefore++;
    });

    // tests
    outerModule.test("foo", function( assert ) {
        assert.equal(this.outerBefore, 1, "beforeEach updated environment");
    });
    outerModule.test("bar", function( assert ) {
        assert.equal(this.outerBefore, 1, "sibling tests don't share environments");
    });

    // submodules
    outerModule.module("inner", function( innerModule ) {
        // environment (inherits from parent)
        this.inner = true;

        function assertEnvironment( assert, before ) {
            assert.ok(this.outer, "outer module initialized environment");
            assert.ok("outerBefore" in this, "outer module beforeEach updated environment");
            assert.equal(this.outerBefore, 1, "submodules don't share environments");
            assert.ok(this.inner, "inner module initialized environment");
            if ( before ) {
                return;
            }
            assert.ok(this.innerBefore, "inner module beforeEach updated environment");
            assert.ok(this.tested, "test updated environment");
        }

        // test before/after
        innerModule.test.beforeEach(function( assert ) {
            this.innerBefore = true;
            assertEnvironment.call( this, assert, true );
        });
        innerModule.test.afterEach(function( assert ) {
            assertEnvironment.call( this, assert );
        });

        // tests
        innerModule.test("bar", function( assert ) {
            this.tested = true;
            assertEnvironment.call( this, assert );
        });
    });
});
@leobalter

This comment has been minimized.

Show comment
Hide comment
@leobalter

leobalter Oct 1, 2015

Member
QUnit.module("outer", function( outerModule ) {
    // environment (prototype for test/submodule context)
    this.outer = true;
    this.outerBefore = 0;

    // test before/after
    outerModule.test.beforeEach(function() {
        this.outerBefore++;
    });

    // tests
    outerModule.test("foo", function( assert ) {
        assert.equal(this.outerBefore, 1, "beforeEach updated environment");
    });
    outerModule.test("bar", function( assert ) {
        assert.equal(this.outerBefore, 1, "sibling tests don't share environments");
    });

    ...

These this.outerBefore tests seems very confusing to me. I can understand the point here but I'm uncomfortable with it so far.


I'm not sure about these particular parameters.

And reviewing my code I'll indeed remove the first test argument.

And following your own example, I would stick with the first argument being an object with the hooks methods and it can later being extended. This way:

QUnit.module("outer", function( outerModule ) {
    ...
    outerModule.beforeEach(function() {
        ...
    });

    outerModule.afterEach(function() {
        ...
    });

but also, later in further additions:

QUnit.module("outer", function( t ) {
    t.beforeEach(function() {
        ...
    });
    t.test( "the test method", function( assert ) {
        ...
    });
});

and using ES6:

QUnit.module("outer", ( { beforeEach } ) => {
    beforeEach(function() {
        ...
    });
    QUnit.test( "the test method", function( assert ) {
        ...
    });
});

The reason to start with beforeEach and afterEach methods is to avoid calling modules with three arguments, where the hooks object is already big enough:

QUnit.module( "outer", {
        beforeEach: function( assert ) {
            ...
        },
        afterEach: function( assert ) {
            ...
        }
    },
    function( t ) {
        QUnit.test( "the test method", function( assert ) {
            ...
        });
    }
);
Member

leobalter commented Oct 1, 2015

QUnit.module("outer", function( outerModule ) {
    // environment (prototype for test/submodule context)
    this.outer = true;
    this.outerBefore = 0;

    // test before/after
    outerModule.test.beforeEach(function() {
        this.outerBefore++;
    });

    // tests
    outerModule.test("foo", function( assert ) {
        assert.equal(this.outerBefore, 1, "beforeEach updated environment");
    });
    outerModule.test("bar", function( assert ) {
        assert.equal(this.outerBefore, 1, "sibling tests don't share environments");
    });

    ...

These this.outerBefore tests seems very confusing to me. I can understand the point here but I'm uncomfortable with it so far.


I'm not sure about these particular parameters.

And reviewing my code I'll indeed remove the first test argument.

And following your own example, I would stick with the first argument being an object with the hooks methods and it can later being extended. This way:

QUnit.module("outer", function( outerModule ) {
    ...
    outerModule.beforeEach(function() {
        ...
    });

    outerModule.afterEach(function() {
        ...
    });

but also, later in further additions:

QUnit.module("outer", function( t ) {
    t.beforeEach(function() {
        ...
    });
    t.test( "the test method", function( assert ) {
        ...
    });
});

and using ES6:

QUnit.module("outer", ( { beforeEach } ) => {
    beforeEach(function() {
        ...
    });
    QUnit.test( "the test method", function( assert ) {
        ...
    });
});

The reason to start with beforeEach and afterEach methods is to avoid calling modules with three arguments, where the hooks object is already big enough:

QUnit.module( "outer", {
        beforeEach: function( assert ) {
            ...
        },
        afterEach: function( assert ) {
            ...
        }
    },
    function( t ) {
        QUnit.test( "the test method", function( assert ) {
            ...
        });
    }
);
@leobalter

This comment has been minimized.

Show comment
Hide comment
@leobalter

leobalter Oct 1, 2015

Member

@gibson042: both 29f5b84 and 53778e3 has the shared this based on your example.

The this object is only propagated from the outer to the inner scopes without duplicating the test hooks.

I also removed the test argument as you can see on the fixup commits.

We may then extend the module callback argument to include test later. I want to at least bring the shared this and the hooks when landing the nested modules.

Member

leobalter commented Oct 1, 2015

@gibson042: both 29f5b84 and 53778e3 has the shared this based on your example.

The this object is only propagated from the outer to the inner scopes without duplicating the test hooks.

I also removed the test argument as you can see on the fixup commits.

We may then extend the module callback argument to include test later. I want to at least bring the shared this and the hooks when landing the nested modules.

var env = {};
if ( parentModule ) {
extend( env, parentModule.testEnvironment );

This comment has been minimized.

@gibson042

gibson042 Oct 1, 2015

Member

I think Object.create-style inheritance would be better, but am willing to shelve that for now.

@gibson042

gibson042 Oct 1, 2015

Member

I think Object.create-style inheritance would be better, but am willing to shelve that for now.

This comment has been minimized.

@leobalter

leobalter Oct 1, 2015

Member

I would love to do this. We still support ES3 until QUnit 2.0.0

@leobalter

leobalter Oct 1, 2015

Member

I would love to do this. We still support ES3 until QUnit 2.0.0

This comment has been minimized.

@gibson042

gibson042 Oct 2, 2015

Member

Arbitrary inheritance is possible in ES3, too:

var createObject = (function( Base ) {
    return function( prototype ) {
        Base.prototype = prototype;
        return new Base();
    };
})( Function() );
…
env = createObject( parentModule.testEnvironment );

The above is similar to a stripped-down version of lodash's baseCreate, but you could easily go all the way to _.create if you want this part to look like module.testEnvironment = createObject( parentModule.testEnvironment, testEnvironment ) (removing the need for a separate extend).

@gibson042

gibson042 Oct 2, 2015

Member

Arbitrary inheritance is possible in ES3, too:

var createObject = (function( Base ) {
    return function( prototype ) {
        Base.prototype = prototype;
        return new Base();
    };
})( Function() );
…
env = createObject( parentModule.testEnvironment );

The above is similar to a stripped-down version of lodash's baseCreate, but you could easily go all the way to _.create if you want this part to look like module.testEnvironment = createObject( parentModule.testEnvironment, testEnvironment ) (removing the need for a separate extend).

This comment has been minimized.

@leobalter

leobalter Oct 2, 2015

Member

This is cool, but I would leave this improvement for later as this requires a whole new review. Does it works for you?

@leobalter

leobalter Oct 2, 2015

Member

This is cool, but I would leave this improvement for later as this requires a whole new review. Does it works for you?

This comment has been minimized.

@gibson042

gibson042 Oct 2, 2015

Member

Yep.

Show outdated Hide outdated test/main/modules.js
} );
hooks.afterEach( function( assert ) {
assert.equal( this.outer, 2 );

This comment has been minimized.

@gibson042

gibson042 Oct 1, 2015

Member

The increment-based tests in leobalter@29f5b84#diff-3ff526461245427b5e9714efeb6ca4ed failed, right? It is unfortunate, but IIRC tests within a module do currently share environment.

@gibson042

gibson042 Oct 1, 2015

Member

The increment-based tests in leobalter@29f5b84#diff-3ff526461245427b5e9714efeb6ca4ed failed, right? It is unfortunate, but IIRC tests within a module do currently share environment.

This comment has been minimized.

@leobalter

leobalter Oct 1, 2015

Member

this commit will be merged with 53778e3.

It is passing here. I just wanted to set the steps on different commits during the review stage.

It is unfortunate, but IIRC tests within a module do currently share environment.

I've done it this way getting from this assertion below from your example.

    // submodules
    outerModule.module("inner", function( innerModule ) {
        // environment (inherits from parent)
        this.inner = true;

        function assertEnvironment( assert, before ) {
            assert.ok(this.outer, "outer module initialized environment"); // <<<<<<<

Should it be different?

@leobalter

leobalter Oct 1, 2015

Member

this commit will be merged with 53778e3.

It is passing here. I just wanted to set the steps on different commits during the review stage.

It is unfortunate, but IIRC tests within a module do currently share environment.

I've done it this way getting from this assertion below from your example.

    // submodules
    outerModule.module("inner", function( innerModule ) {
        // environment (inherits from parent)
        this.inner = true;

        function assertEnvironment( assert, before ) {
            assert.ok(this.outer, "outer module initialized environment"); // <<<<<<<

Should it be different?

This comment has been minimized.

@gibson042

gibson042 Oct 2, 2015

Member

Turns out the environments are already distinct (🙌), so there's just some missing code here to show that in-test environment modifications are visible by afterEach callbacks but not by subsequent tests (as demonstrated in leobalter@29f5b84#diff-3ff526461245427b5e9714efeb6ca4ed ).

@gibson042

gibson042 Oct 2, 2015

Member

Turns out the environments are already distinct (🙌), so there's just some missing code here to show that in-test environment modifications are visible by afterEach callbacks but not by subsequent tests (as demonstrated in leobalter@29f5b84#diff-3ff526461245427b5e9714efeb6ca4ed ).

This comment has been minimized.

@leobalter

leobalter Oct 2, 2015

Member

Good catch. I got it on ce70ce9

@leobalter

leobalter Oct 2, 2015

Member

Good catch. I got it on ce70ce9

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Oct 1, 2015

Member

👍 The single callback argument is a decent (if limited) module stand-in and has good room for growth, and you are absolutely right about this:

The reason to start with beforeEach and afterEach methods is to avoid calling modules with three arguments, where the hooks object is already big enough

Let's hope that module and test follow suit, so that we can eventually get to a point where module callbacks need nothing other than what's passed directly to them, just like current test callbacks.

Member

gibson042 commented Oct 1, 2015

👍 The single callback argument is a decent (if limited) module stand-in and has good room for growth, and you are absolutely right about this:

The reason to start with beforeEach and afterEach methods is to avoid calling modules with three arguments, where the hooks object is already big enough

Let's hope that module and test follow suit, so that we can eventually get to a point where module callbacks need nothing other than what's passed directly to them, just like current test callbacks.

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Oct 5, 2015

Member

I think this is good to go; module environment inheritance can be addressed with a subsequent issue & PR.

Member

gibson042 commented Oct 5, 2015

I think this is good to go; module environment inheritance can be addressed with a subsequent issue & PR.

@leobalter

This comment has been minimized.

Show comment
Hide comment
@leobalter

leobalter Oct 5, 2015

Member

Opened #869 to follow up with this.

Member

leobalter commented Oct 5, 2015

Opened #869 to follow up with this.

leobalter added a commit to qunitjs/api that referenced this pull request Oct 7, 2015

leobalter added a commit to leobalter/api.qunitjs.com that referenced this pull request Oct 7, 2015

Askelkana and others added some commits Mar 25, 2015

Core: Nested modules
Allows modules to be nested. This is achieved by adding a function
argument to the QUnit.module method, which is a function that is called
for immediate processing. Also, testEnvironments for parent modules
are invoked in recursively for each test (outer-first).

Fixes #543
Closes #800
Closes #859
Closes #670
Ref #858

@leobalter leobalter closed this in 7be1d6c Oct 8, 2015

leobalter added a commit that referenced this pull request Oct 8, 2015

leobalter added a commit that referenced this pull request Oct 8, 2015

@leobalter leobalter deleted the leobalter:nested-modules branch Oct 8, 2015

leobalter added a commit to qunitjs/api that referenced this pull request Oct 27, 2015

leobalter added a commit to qunitjs/api that referenced this pull request Oct 27, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment