Core: Nested modules #800

Askelkana · 2015-03-25T17:25:14Z

Allows nesting of modules by enabling passing function as third argument
to QUnit.module method.

Fixes #543

Askelkana · 2015-03-25T17:26:49Z

I signed the CLA prior to creating the pull request (losing my virginity with this). Don't know what happens after that...

arthurvr · 2015-03-25T17:28:41Z

@Askelkana You used your username when signing the CLA, not your full name.

Askelkana · 2015-03-25T18:37:16Z

Actually, I didn't. But I'll have another go...

arthurvr · 2015-03-25T18:42:28Z

@Askelkana Now the CLA appears to be signed with your full name, thanks. You'll have to update your git config to include your full name as well. Normally it should be fine then.

Askelkana · 2015-03-25T20:12:57Z

By way of interest, the CLA page reads as follows:

The email address used to sign the CLA must match the email address in your git config. The author information for each commit in your Contributions will be cross-checked with this signature.

It does not mention that the name given is significant.

Allows nesting of modules by enabling passing function as third argument to QUnit.module method. Fixes #543

Askelkana · 2015-03-25T20:25:59Z

Ok, git config updated and commited. I assume the CI build fail is not critical, since there are several errors being thrown which have nothing to do with my changes.

leobalter · 2015-03-26T03:59:20Z

I'll review this tomorrow morning but you also need to run grunt in order to lint and check the code style.

This PR looks interesting, I'm looking forward to see it passing the on CI.

leobalter · 2015-03-26T14:50:42Z

current tests are working, but I also made this following test in your branch:

QUnit.module( "multiple hooks", {
    beforeEach: function( assert ) {
        assert.ok( true, "root module beforeEach" );
    },
    afterEach: function( assert ) {
        assert.ok( true, "root module afterEach" );
    }
}, function() {
    QUnit.module( "nested module", {
        beforeEach: function( assert ) {
            assert.ok( true, "nested module beforeEach" );
        },
        afterEach: function( assert ) {
            assert.ok( true, "nested module afterEach" );
        }
    });

    QUnit.test( "all hooks must be called", function( assert ) {
        assert.expect( 4 );
    });
});

It failed as it only triggered the nested module hooks. This test should pass triggering all the chained modules' hooks.

IMHO, the only real advantage on having nested modules is to stack the hooks. Otherwise it would just create a fancy report.

cc @JamesMGreene.

Askelkana · 2015-03-26T16:50:20Z

I think your tests are incorrect: surely the afterEach would be called after the test has run, and therefore any assertions fired there will not be caught by the expect method. However, I have integrated the intent of the check into my original tests.

I wanted to wait for feedback before attempting this, even though it was mentioned in the original issue's comments. So thanks for the confirmation.

I would add that another advantage to nesting modules is that it clears up one's testing code no end, since it allows for much better arrangement of the tests.

I'll push a commit through soon.

leobalter · 2015-03-26T16:57:37Z

the expect also counts the assertions in the following afterEach hook.

Some other issues I noticed:

the nested modules are not listed on the HTML Reporter
selecting the root modules on the HTML Reporter filter doesn't run the tests on its respective nested modules

Allows modules to be nested. This is achieved by adding a third possible 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

Askelkana · 2015-03-26T17:20:58Z

You are right. Tests still holding.

As for the HTMLReporter, at present the modules are not highlighted in any way. I didn't wish to presume how this should be handled, so I plumbed for simply showing the module path on the test row, as before.

I agree that ideally, some nesting mechanism should be portrayed by the reporter.

leobalter · 2015-03-26T17:28:01Z

src/core.js

-		for ( key in obj ) {
-			if ( hasOwn.call( obj, key ) ) {
+		for (key in obj) {
+			if (hasOwn.call( obj, key )) {


something is breaking the code style, and it's becoming hard to check the real diffs here.

Ok, I'll check it out. Everything passed the jshint and jscs tests, so I didn't notice these.

The actual changes this time are in the test.run and test.hook methods.

Allows modules to be nested. This is achieved by adding a third possible 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

Askelkana · 2015-03-26T18:27:43Z

the nested modules are not listed on the HTML Reporter

Well, it depends how you want them displayed. The "concatenated" list of a test's module ancestry is certainly given. Do we want some nesting functionality? Perhaps not.

Allows modules to be nested. This is achieved by adding a third possible 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

Askelkana · 2015-03-27T16:01:02Z

Ok, I just pushed an addition that all tests belonging to a super-module are run when selecting the said module from the filter. Unless you have any other issues, I think this is good to go.

jzaefferer · 2015-04-09T17:06:48Z

src/core.js

@@ -2,14 +2,15 @@ var QUnit,
 	config,
 	onErrorFnPrev,
 	loggingCallbacks = {},
-	fileName = ( sourceFromStacktrace( 0 ) || "" ).replace( /(:\d+)+\)?/, "" ).replace( /.+\//, "" ),
+	fileName = ( sourceFromStacktrace( 0 ) || "" ).replace( /(:\d+)+\)?/, "" ).replace( /.+\//,


There are lots of whitespace or linebreak changes like this. They don't belong in this PR, most of them seem just plain wrong. Can you address that? We can then do a proper review (more timely, hopefully).

Allows modules to be nested. This is achieved by adding a third possible 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

Askelkana · 2015-04-22T18:32:14Z

Apologies for the delay. Slowed by poor health and a garden to get ready for planting...

Anyway, I've fixed (or think I have) the style issues. I take your point about not trying to solve more than one problem at a time. But the code really could stand some cleaning: it's quite inconsistent in many cases, and certainly doesn't pass the jslint test.

Allows modules to be nested. This is achieved by adding a third possible 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

Askelkana · 2015-04-22T18:36:22Z

Yes, all styles issues have now been resolved. Happy reviewing!

jzaefferer · 2015-09-10T15:32:53Z

@leobalter what do you think?

leobalter · 2015-09-10T16:04:25Z

I believe fixing the conflict issues won't be too hard.

I'll merge this and then I'll propose an extra PR to make the QUnit.module functions to come with the test and QUnit as the arguments.

The test argument is just to have it as a local reference (avoiding globals without using namespaces, aka QUnit.test); The QUnit argument is to allow access to additional features, as QUnit.skip, QUnit.config, the proposed QUnit.only, etc.

A second following PR proposal is to also alias QUnit.module to the QUnit. This is more insane, but would allow a simple and dynamic use with these nested modules:

import QUnit from 'qunitjs';

QUnit( "my first batch", ( test ) => {
  test( "test this", ( t ) => {
    t.ok( true, "it pass" );
  } );
} );

or

import suite from 'qunitjs';

suite( "my first batch", ( test ) => {
  test( "test this", ( t ) => {
    t.ok( true, "it pass" );
  } );
} );

I'll open the respective issues to discuss them as soon as I merge this PR.

gibson042 · 2015-09-10T17:38:01Z

This PR exhibits some potentially surprising behaviors:

executing all of a module's own tests before entering its submodules (regardless of test/submodule definition order)
always minting a new environment definition for every submodule, even if its hooks argument is undefined or not even specified

If they are intentional, there should be assertions covering them. If they are unintentional and undesired, there should be assertions covering the desired behavior.

leobalter · 2015-09-10T18:30:21Z

The squashed and rebased branch is here: https://github.com/jquery/qunit/compare/jquery:master...leobalter:nested-modules?expand=1

@gibson042

executing all of a module's own tests before entering its submodules (regardless of test/submodule definition order)

This might seem weird, but with the idea of each test should be atomic, an specific order shouldn't be a major concern. The real world prevents me to keep this argument as this will indeed blow out eventually without assertions with the intentional behavior.

always minting a new environment definition for every submodule, even if its hooks argument is undefined or not even specified

I don't see it clearly, but I believe this and along with the other behavior could be improved in a next PR?

The main feature seems ok and working fine, and I am late giving the proper attention to this work. So if you agree, I would let these parts to be treated in a next commit bit.

gibson042 · 2015-09-10T18:56:55Z

The main feature seems ok and working fine, and I am late giving the proper attention to this work. So if you agree, I would let these parts to be treated in a next commit bit.

👍

I just didn't want to lose these thoughts (or the one regarding explicit context sensitivity of beforeEach/afterEach/test/etc. via arguments or context, which should be covered by your above followup).

leobalter · 2015-09-10T20:13:14Z

test/modules.js

+			var module = assert.test.module;
+			assert.equal( module.name, "nested modules > second outer" );
+		});
+});


I found an interesting and possibly blocking behavior:

QUnit.module calls with functions are not affecting the following tests written outside the function.

Like this following example right after this line where I am commenting:

QUnit.test( "test foo", function( a ) { a.equal( a.test.module.name, "nested modules" ); } );

This test is failing, telling the actual value is "Deprecated setup/teardown", referencing the module written above "nested modules".

@jzaefferer The implementation is tied to this behavior.

I believe this can still be shipped but mentioning that calling QUnit.module with a nested function for tests will not spread the current module for consequent QUnit.test our of its nested function.

This should be tested, documented and we might want to deprecate the usage of QUnit.module without a function parameter.

also: calling QUnit.module without a function inside a QUnit.module function will cause a strong mess with the nested behavior. That's a reason it should be deprecated.

please, see #859

`QUnit.module` calls with functions are not affecting the following tests written outside the function. Ref qunitjs#800 (comment)

leobalter · 2015-10-01T22:41:13Z

Closing this, further work in progress on #859

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 qunitjs#543 Closes qunitjs#800 Closes qunitjs#859 Closes qunitjs#670 Ref qunitjs#858

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

jquerybot added the CLA: Error label Mar 25, 2015

Core: Nested modules

a68adb0

Allows nesting of modules by enabling passing function as third argument to QUnit.module method. Fixes #543

jquerybot added CLA: Valid and removed CLA: Error labels Mar 26, 2015

leobalter reviewed Mar 26, 2015
View reviewed changes

jzaefferer reviewed Apr 9, 2015
View reviewed changes

leobalter mentioned this pull request Sep 10, 2015

document nested modules qunitjs/api.qunitjs.com#109

Closed

leobalter mentioned this pull request Sep 10, 2015

Nested modules improvements #858

Closed

leobalter reviewed Sep 10, 2015
View reviewed changes

leobalter mentioned this pull request Sep 10, 2015

Nested modules #859

Closed

leobalter closed this Oct 1, 2015

gibson042 mentioned this pull request Nov 19, 2015

Allow testEnvironment to be accessed within a test without this #894

Closed

gibson042 mentioned this pull request Jan 29, 2016

Separate module options from test environment #923

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Core: Nested modules #800

Core: Nested modules #800

Askelkana commented Mar 25, 2015

Askelkana commented Mar 25, 2015

arthurvr commented Mar 25, 2015

Askelkana commented Mar 25, 2015

arthurvr commented Mar 25, 2015

Askelkana commented Mar 25, 2015

Askelkana commented Mar 25, 2015

leobalter commented Mar 26, 2015

leobalter commented Mar 26, 2015

Askelkana commented Mar 26, 2015

leobalter commented Mar 26, 2015

Askelkana commented Mar 26, 2015

leobalter Mar 26, 2015

Askelkana Mar 26, 2015

Askelkana commented Mar 26, 2015

Askelkana commented Mar 27, 2015

jzaefferer Apr 9, 2015

Askelkana commented Apr 22, 2015

Askelkana commented Apr 22, 2015

jzaefferer commented Sep 10, 2015

leobalter commented Sep 10, 2015

gibson042 commented Sep 10, 2015

leobalter commented Sep 10, 2015

gibson042 commented Sep 10, 2015

leobalter Sep 10, 2015

leobalter Sep 10, 2015

leobalter Sep 10, 2015

leobalter Sep 10, 2015

leobalter commented Oct 1, 2015

Core: Nested modules #800

Core: Nested modules #800

Conversation

Askelkana commented Mar 25, 2015

Askelkana commented Mar 25, 2015

arthurvr commented Mar 25, 2015

Askelkana commented Mar 25, 2015

arthurvr commented Mar 25, 2015

Askelkana commented Mar 25, 2015

Askelkana commented Mar 25, 2015

leobalter commented Mar 26, 2015

leobalter commented Mar 26, 2015

Askelkana commented Mar 26, 2015

leobalter commented Mar 26, 2015

Askelkana commented Mar 26, 2015

leobalter Mar 26, 2015

Choose a reason for hiding this comment

Askelkana Mar 26, 2015

Choose a reason for hiding this comment

Askelkana commented Mar 26, 2015

Askelkana commented Mar 27, 2015

jzaefferer Apr 9, 2015

Choose a reason for hiding this comment

Askelkana commented Apr 22, 2015

Askelkana commented Apr 22, 2015

jzaefferer commented Sep 10, 2015

leobalter commented Sep 10, 2015

gibson042 commented Sep 10, 2015

leobalter commented Sep 10, 2015

gibson042 commented Sep 10, 2015

leobalter Sep 10, 2015

Choose a reason for hiding this comment

leobalter Sep 10, 2015

Choose a reason for hiding this comment

leobalter Sep 10, 2015

Choose a reason for hiding this comment

leobalter Sep 10, 2015

Choose a reason for hiding this comment

leobalter commented Oct 1, 2015