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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tests: Allowed nested suites/modules #670

Closed
wants to merge 2 commits into
base: master
from

Conversation

7 participants
@JamesMGreene
Member

JamesMGreene commented Sep 25, 2014

Fixes #543
Closes #670

Debatably a work-in-progress... but... it works pretty well so far! 馃憤

For the sake of comparison (and so I didn't go crazy), I chose to implement this functionality in a separate API method, QUnit.suite, rather than trying to make QUnit.module work in 2 very different ways. They can conceivably be combined, though.

Another note of interest in how I chose to make suites work differently than modules: every test inside of a suite gets its own atomic context object that can be modified along the way by every level's beforeEach/afterEach calls. Conversely, using modules, every test within that module shares a shallow copy of the same context object.

@JamesMGreene

This comment has been minimized.

Show comment
Hide comment
@JamesMGreene

JamesMGreene Sep 25, 2014

Member

@dcherman @gibson042 @mmun @stefanpenner: This may be of interest to you. 馃挭

Member

JamesMGreene commented Sep 25, 2014

@dcherman @gibson042 @mmun @stefanpenner: This may be of interest to you. 馃挭

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner
Contributor

stefanpenner commented Sep 25, 2014

@dcherman

This comment has been minimized.

Show comment
Hide comment
@dcherman

dcherman commented Sep 25, 2014

鉂わ笍

@JamesMGreene

This comment has been minimized.

Show comment
Hide comment
@JamesMGreene

JamesMGreene Sep 28, 2014

Member

Very quiet here... making me nervous. 馃槄

Member

JamesMGreene commented Sep 28, 2014

Very quiet here... making me nervous. 馃槄

@JamesMGreene JamesMGreene modified the milestone: v2.0 Sep 28, 2014

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Sep 29, 2014

Member

Sheesh; give us a minute. 馃榾

Is this intended primarily to demonstrate implementation of previously-agreed API changes, or a possibility for said API changes? I ask because @Krinkle grabbed me in Chicago for some noodling on an assertion grouping API that was recursive, flexible, and intuitive, but still "felt" like QUnit. I don't think he's shared the result yet, but here's the gist: https://gist.github.com/Krinkle/8d696b013ba99ff1cc0d#file-20-unified-test-group-js.

The basic idea there was making test the sole grouping abstraction, able to contain assertions and tests and thereby eliminating the need for a higher level like module. Passing test as an argument allows readability through parameter naming (which could be used in a simple fashion to separate module from test, and in a more advanced way to support e.g. BDD syntax). Also, test itself then proves to be a convenient object upon which to hang beforeEach/afterEach now, and in the future possible extensions like fixture/mock/etc.

Member

gibson042 commented Sep 29, 2014

Sheesh; give us a minute. 馃榾

Is this intended primarily to demonstrate implementation of previously-agreed API changes, or a possibility for said API changes? I ask because @Krinkle grabbed me in Chicago for some noodling on an assertion grouping API that was recursive, flexible, and intuitive, but still "felt" like QUnit. I don't think he's shared the result yet, but here's the gist: https://gist.github.com/Krinkle/8d696b013ba99ff1cc0d#file-20-unified-test-group-js.

The basic idea there was making test the sole grouping abstraction, able to contain assertions and tests and thereby eliminating the need for a higher level like module. Passing test as an argument allows readability through parameter naming (which could be used in a simple fashion to separate module from test, and in a more advanced way to support e.g. BDD syntax). Also, test itself then proves to be a convenient object upon which to hang beforeEach/afterEach now, and in the future possible extensions like fixture/mock/etc.

@JamesMGreene

This comment has been minimized.

Show comment
Hide comment
@JamesMGreene

JamesMGreene Sep 29, 2014

Member

Is this intended primarily to demonstrate implementation of previously-agreed API changes, or a possibility for said API changes?

The latter, I suppose: prove that it is actually fairly trivial to implement the same nested functionality provided by pretty much every other JS unit testing framework. This also provides basically the same non-verbose API as all of the others, when coupled with the simple IIFE I typically use to make a few important QUnit top-level properties (in this case: QUnit.suite, QUnit.test, QUnit.beforeEach, QUnit.afterEach) look globally accessible within it.

I talked with @Krinkle in Chicago about this stuff as well but he and I seem to have differing opinions here. His primary aim seems to be providing an API that is different and/or more explicit than other JS unit testing framework, whereas I prefer to arrive at the same end without the explicit/verbose nature of having to pass/expect/chain more arguments/parameters than truly necessary (which is really just assert, IMHO).

From the Gist, I could probably accept the API in example "10" ("context") but I think the more extreme version in "20" ("unified test group"), while I understand it, would easily confuse and/or alienate both existing and potential new consumers of the library... at least without using better terminology, examples, and documentation to explain how to use it (i.e. "there are only two things: assertions, and groups of assertions and/or other groups"). Having well-defined concepts for assertions, tests (groups of assertions), and suites (groups of tests and/or other suites) is a standard concept in every unit testing framework that I've ever seen or heard of.

@Krinkle's "-1 concept" approach will assuredly work but it will seem foreign to pretty much everyone.

Member

JamesMGreene commented Sep 29, 2014

Is this intended primarily to demonstrate implementation of previously-agreed API changes, or a possibility for said API changes?

The latter, I suppose: prove that it is actually fairly trivial to implement the same nested functionality provided by pretty much every other JS unit testing framework. This also provides basically the same non-verbose API as all of the others, when coupled with the simple IIFE I typically use to make a few important QUnit top-level properties (in this case: QUnit.suite, QUnit.test, QUnit.beforeEach, QUnit.afterEach) look globally accessible within it.

I talked with @Krinkle in Chicago about this stuff as well but he and I seem to have differing opinions here. His primary aim seems to be providing an API that is different and/or more explicit than other JS unit testing framework, whereas I prefer to arrive at the same end without the explicit/verbose nature of having to pass/expect/chain more arguments/parameters than truly necessary (which is really just assert, IMHO).

From the Gist, I could probably accept the API in example "10" ("context") but I think the more extreme version in "20" ("unified test group"), while I understand it, would easily confuse and/or alienate both existing and potential new consumers of the library... at least without using better terminology, examples, and documentation to explain how to use it (i.e. "there are only two things: assertions, and groups of assertions and/or other groups"). Having well-defined concepts for assertions, tests (groups of assertions), and suites (groups of tests and/or other suites) is a standard concept in every unit testing framework that I've ever seen or heard of.

@Krinkle's "-1 concept" approach will assuredly work but it will seem foreign to pretty much everyone.

@JamesMGreene

This comment has been minimized.

Show comment
Hide comment
@JamesMGreene

JamesMGreene Sep 29, 2014

Member

The "unified test group" concept is also not very compatible with any Reporter that comes to mind. What's a "test"? What's a "suite"? How do we track each of them effectively without making our code overly complex to somehow distinguish between them?

Member

JamesMGreene commented Sep 29, 2014

The "unified test group" concept is also not very compatible with any Reporter that comes to mind. What's a "test"? What's a "suite"? How do we track each of them effectively without making our code overly complex to somehow distinguish between them?

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Sep 29, 2014

Contributor

@JamesMGreene sorry i haven't had time to review yet, but this functionality makes me happy. I will try to look into it later this week.

Contributor

stefanpenner commented Sep 29, 2014

@JamesMGreene sorry i haven't had time to review yet, but this functionality makes me happy. I will try to look into it later this week.

@JamesMGreene

This comment has been minimized.

Show comment
Hide comment
@JamesMGreene

JamesMGreene Sep 30, 2014

Member

Don't mind me, I'm just PR-impatient. 馃榾

Member

JamesMGreene commented Sep 30, 2014

Don't mind me, I'm just PR-impatient. 馃榾

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Sep 30, 2014

Member

I talked with @Krinkle in Chicago about this stuff as well but he and I seem to have differing opinions here. His primary aim seems to be providing an API that is different and/or more explicit than other JS unit testing framework

I largely agreed with @Krinkle, but I'm interested in digging deeper here.

I prefer to arrive at the same end without the explicit/verbose nature of having to pass/expect/chain more arguments/parameters than truly necessary (which is really just assert, IMHO).

Can you elaborate? I'd like assert to bundle only assertions, and in fact the assert.suite introduced here seems... misplaced.

From the Gist, I could probably accept the API in example "10" ("context") but I think the more extreme version in "20" ("unified test group"), while I understand it, would easily confuse and/or alienate both existing and potential new consumers of the library... at least without using better terminology, examples, and documentation to explain how to use it (i.e. "there are only two things: assertions, and groups [of assertions and/or other groups]").

That looks great. Perhaps also: "groups containing other groups are also known as 'suites' or 'modules' in other test frameworks, including QUnit before version 2.0".

Having well-defined concepts for assertions, tests (groups of assertions), and suites (groups of tests and/or other suites) is a standard concept in every unit testing framework that I've ever seen or heard of.

Isn't that more complicated, though? It certainly makes more work when I want to carve out a new subtest (e.g., replacing the containing test with module). And for what benefit?

@Krinkle's "-1 concept" approach will assuredly work but it will seem foreign to pretty much everyone.

I think that's underestimating the audience.

The "unified test group" concept is also not very compatible with any Reporter that comes to mind. What's a "test"? What's a "suite"?

TAP

1..3
ok 1 - First test
    # Subtest: An example subtest
    1..2
    ok 1 - This is a subtest
    ok 2 - So is this
ok 2 - An example subtest
ok 3 - Third test

HTML

<li class="fail">"suite"<ol>
  <li class="pass">"test"<ol>
    <li class="pass">assertion</li>
    <li class="pass">assertion</li>
  </ol></li>
  <li class="fail">"test"<ol>
    <li class="pass">assertion</li>
    <li class="fail">assertion</li>
  </ol></li>
</ol></li>

Runtime
Refer to #531 (comment), but possibilities include:

  • No "suite"
  • Every group maps to "suite"
  • Outermost-level group maps to "suite"

How do we track each of them effectively without making our code overly complex to somehow distinguish between them?

What do you mean?

Member

gibson042 commented Sep 30, 2014

I talked with @Krinkle in Chicago about this stuff as well but he and I seem to have differing opinions here. His primary aim seems to be providing an API that is different and/or more explicit than other JS unit testing framework

I largely agreed with @Krinkle, but I'm interested in digging deeper here.

I prefer to arrive at the same end without the explicit/verbose nature of having to pass/expect/chain more arguments/parameters than truly necessary (which is really just assert, IMHO).

Can you elaborate? I'd like assert to bundle only assertions, and in fact the assert.suite introduced here seems... misplaced.

From the Gist, I could probably accept the API in example "10" ("context") but I think the more extreme version in "20" ("unified test group"), while I understand it, would easily confuse and/or alienate both existing and potential new consumers of the library... at least without using better terminology, examples, and documentation to explain how to use it (i.e. "there are only two things: assertions, and groups [of assertions and/or other groups]").

That looks great. Perhaps also: "groups containing other groups are also known as 'suites' or 'modules' in other test frameworks, including QUnit before version 2.0".

Having well-defined concepts for assertions, tests (groups of assertions), and suites (groups of tests and/or other suites) is a standard concept in every unit testing framework that I've ever seen or heard of.

Isn't that more complicated, though? It certainly makes more work when I want to carve out a new subtest (e.g., replacing the containing test with module). And for what benefit?

@Krinkle's "-1 concept" approach will assuredly work but it will seem foreign to pretty much everyone.

I think that's underestimating the audience.

The "unified test group" concept is also not very compatible with any Reporter that comes to mind. What's a "test"? What's a "suite"?

TAP

1..3
ok 1 - First test
    # Subtest: An example subtest
    1..2
    ok 1 - This is a subtest
    ok 2 - So is this
ok 2 - An example subtest
ok 3 - Third test

HTML

<li class="fail">"suite"<ol>
  <li class="pass">"test"<ol>
    <li class="pass">assertion</li>
    <li class="pass">assertion</li>
  </ol></li>
  <li class="fail">"test"<ol>
    <li class="pass">assertion</li>
    <li class="fail">assertion</li>
  </ol></li>
</ol></li>

Runtime
Refer to #531 (comment), but possibilities include:

  • No "suite"
  • Every group maps to "suite"
  • Outermost-level group maps to "suite"

How do we track each of them effectively without making our code overly complex to somehow distinguish between them?

What do you mean?

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Oct 2, 2014

Member

One thing I noticed: The diff in this PR has no occurrence of "async". Especially in the context of the kinda-global before/afterEach method that seems like a big mistake.

Otherwise, as mentioned elsewhere, I don't want this to block 2.0, so I'm mostly focused on the remaining pre-2.0 tasks right now.

Member

jzaefferer commented Oct 2, 2014

One thing I noticed: The diff in this PR has no occurrence of "async". Especially in the context of the kinda-global before/afterEach method that seems like a big mistake.

Otherwise, as mentioned elsewhere, I don't want this to block 2.0, so I'm mostly focused on the remaining pre-2.0 tasks right now.

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Oct 3, 2014

Member

I had a further thought that makes me strongly lean towards this option:

Outermost-level group maps to "suite"

Specifically, the module picker, but generally, the fact that only the outermost groups are known before execution starts. And in that sense, I'm even more inclined to point out that what's being offered is not so much "nested suites" as "nested tests".

Member

gibson042 commented Oct 3, 2014

I had a further thought that makes me strongly lean towards this option:

Outermost-level group maps to "suite"

Specifically, the module picker, but generally, the fact that only the outermost groups are known before execution starts. And in that sense, I'm even more inclined to point out that what's being offered is not so much "nested suites" as "nested tests".

@leobalter

This comment has been minimized.

Show comment
Hide comment
@leobalter

leobalter Dec 8, 2014

Member

I would like to have this one landed for the next version. Even if it's still a 1.x, we could have our first impressions on using the nested suites.

Member

leobalter commented Dec 8, 2014

I would like to have this one landed for the next version. Even if it's still a 1.x, we could have our first impressions on using the nested suites.

@albell

This comment has been minimized.

Show comment
Hide comment
@albell

albell Jan 22, 2015

+1 for infinite-depth nesting. Excited to use this. Thanks guys.

albell commented Jan 22, 2015

+1 for infinite-depth nesting. Excited to use this. Thanks guys.

@JamesMGreene

This comment has been minimized.

Show comment
Hide comment
@JamesMGreene

JamesMGreene Mar 11, 2015

Member

@gibson042

Specifically, the module picker, but generally, the fact that only the outermost groups are known before execution starts. And in that sense, I'm even more inclined to point out that what's being offered is not so much "nested suites" as "nested tests".

Actually, that's not true here. The callback param passed to the QUnit.suite method is invoked immediately (and synchronously), so we could build a whole hierarchy of "modules"/"suites" to put in the module/suite filter dropdown before the test run even starts.

Member

JamesMGreene commented Mar 11, 2015

@gibson042

Specifically, the module picker, but generally, the fact that only the outermost groups are known before execution starts. And in that sense, I'm even more inclined to point out that what's being offered is not so much "nested suites" as "nested tests".

Actually, that's not true here. The callback param passed to the QUnit.suite method is invoked immediately (and synchronously), so we could build a whole hierarchy of "modules"/"suites" to put in the module/suite filter dropdown before the test run even starts.

@JamesMGreene

This comment has been minimized.

Show comment
Hide comment
@JamesMGreene

JamesMGreene Mar 11, 2015

Member

I would like to have this one landed for the next version. Even if it's still a 1.x, we could have our first impressions on using the nested suites.

I agree with @leobalter on this, plus adding this into a late v1.x release gives us an easy SemVer "out" to change/remove the API if needed in v2.x. Should probably mark this API as "experimental" or "unstable" in the v1.x docs, though.

Member

JamesMGreene commented Mar 11, 2015

I would like to have this one landed for the next version. Even if it's still a 1.x, we could have our first impressions on using the nested suites.

I agree with @leobalter on this, plus adding this into a late v1.x release gives us an easy SemVer "out" to change/remove the API if needed in v2.x. Should probably mark this API as "experimental" or "unstable" in the v1.x docs, though.

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Mar 12, 2015

Member

The callback param passed to the QUnit.suite method is invoked immediately (and synchronously), so we could build a whole hierarchy of "modules"/"suites" to put in the module/suite filter dropdown before the test run even starts.

Oh, of course! It's not that code passed to test is necessarily limited to assertions, but it is necessarily asynchronously invoked and optional. And coming from the other side, code passed to module/suite need not necessarily lack assertions, but any assertions would necessarily execute synchronously and precede any setup/beforeEach code. So they are conceptually distinct, and with that in mind I withdraw my support for unification at this time鈥攂ut would still like to preserve it as a future option, which will be important when evaluating invocation context and arguments.

So on that note...

every test inside of a suite gets its own atomic context object that can be modified along the way by every level's beforeEach/afterEach calls. Conversely, using modules, every test within that module shares a shallow copy of the same context object.

That's the this object for test, beforeEach and afterEach code, right? I like the sound of this, but can you elaborate on the differences?

This also provides basically the same non-verbose API as all of the others, when coupled with the simple IIFE I typically use to make a few important QUnit top-level properties (in this case: QUnit.suite, QUnit.test, QUnit.beforeEach, QUnit.afterEach) look globally accessible within it.

I think it's out of scope for this PR, but I'd really favor explicitly contextual versions of those, for reasons similar to those that motivated explicitly contextual assertions. There's a hint of it in the last paragraph of #670 (comment) .

And a bump on this topic, which I think was lost in the noise: #670 (comment)

I prefer to arrive at the same end without the explicit/verbose nature of having to pass/expect/chain more arguments/parameters than truly necessary (which is really just assert, IMHO).

Can you elaborate? I'd like assert to bundle only assertions, and in fact the assert.suite introduced here seems... misplaced.

My (very late) epiphany has definitely warmed me to this approach, though. Thanks for the clarification, @JamesMGreene.

Member

gibson042 commented Mar 12, 2015

The callback param passed to the QUnit.suite method is invoked immediately (and synchronously), so we could build a whole hierarchy of "modules"/"suites" to put in the module/suite filter dropdown before the test run even starts.

Oh, of course! It's not that code passed to test is necessarily limited to assertions, but it is necessarily asynchronously invoked and optional. And coming from the other side, code passed to module/suite need not necessarily lack assertions, but any assertions would necessarily execute synchronously and precede any setup/beforeEach code. So they are conceptually distinct, and with that in mind I withdraw my support for unification at this time鈥攂ut would still like to preserve it as a future option, which will be important when evaluating invocation context and arguments.

So on that note...

every test inside of a suite gets its own atomic context object that can be modified along the way by every level's beforeEach/afterEach calls. Conversely, using modules, every test within that module shares a shallow copy of the same context object.

That's the this object for test, beforeEach and afterEach code, right? I like the sound of this, but can you elaborate on the differences?

This also provides basically the same non-verbose API as all of the others, when coupled with the simple IIFE I typically use to make a few important QUnit top-level properties (in this case: QUnit.suite, QUnit.test, QUnit.beforeEach, QUnit.afterEach) look globally accessible within it.

I think it's out of scope for this PR, but I'd really favor explicitly contextual versions of those, for reasons similar to those that motivated explicitly contextual assertions. There's a hint of it in the last paragraph of #670 (comment) .

And a bump on this topic, which I think was lost in the noise: #670 (comment)

I prefer to arrive at the same end without the explicit/verbose nature of having to pass/expect/chain more arguments/parameters than truly necessary (which is really just assert, IMHO).

Can you elaborate? I'd like assert to bundle only assertions, and in fact the assert.suite introduced here seems... misplaced.

My (very late) epiphany has definitely warmed me to this approach, though. Thanks for the clarification, @JamesMGreene.

execHistory.push( assert.suite.fullName + " -> test2.callback" );
});
suite( "level2", function() {

This comment has been minimized.

@gibson042

gibson042 Mar 12, 2015

Member

Tying invocation of global beforeEach/afterEach/test/suite functions to suite context by virtue of being executed during a suite callback feels dirty (and exactly like tying invocation of global assertion functions to test context by virtue of being executed during a test callback, which is deservedly on the way out). I know there are backcompat reasons for that behavior with module, but I think we can do better with this suite green field.

I'd like them to instead be accessible from either context or arguments, which directly relates to (and partly refutes) the end of #670 (comment) . How to do so may be a matter of great bikeshedding, though. Some of my own goals on that point are as follows, in no particular order:

  • Intuitively-readable invocations, possibly paradigm-dependent (e.g., use of BDD names)
  • Maximum similarity between suite and test callbacks, for future iterative convergence
  • Separate concerns (e.g., no properties on assert that do not pertain to assertions, since we might want to allow integrating external assertion libraries)
@gibson042

gibson042 Mar 12, 2015

Member

Tying invocation of global beforeEach/afterEach/test/suite functions to suite context by virtue of being executed during a suite callback feels dirty (and exactly like tying invocation of global assertion functions to test context by virtue of being executed during a test callback, which is deservedly on the way out). I know there are backcompat reasons for that behavior with module, but I think we can do better with this suite green field.

I'd like them to instead be accessible from either context or arguments, which directly relates to (and partly refutes) the end of #670 (comment) . How to do so may be a matter of great bikeshedding, though. Some of my own goals on that point are as follows, in no particular order:

  • Intuitively-readable invocations, possibly paradigm-dependent (e.g., use of BDD names)
  • Maximum similarity between suite and test callbacks, for future iterative convergence
  • Separate concerns (e.g., no properties on assert that do not pertain to assertions, since we might want to allow integrating external assertion libraries)

@gibson042 gibson042 referenced this pull request Sep 10, 2015

Closed

Core: Nested modules #800

leobalter added a commit to leobalter/qunit that referenced this pull request Oct 7, 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment