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

Decouple the "root" property on Suite from title length. #2755

Closed
lddubeau opened this issue Mar 29, 2017 · 0 comments · Fixed by #3632
Closed

Decouple the "root" property on Suite from title length. #2755

lddubeau opened this issue Mar 29, 2017 · 0 comments · Fixed by #3632
Labels
area: usability concerning user experience or interface type: feature enhancement proposal

Comments

@lddubeau
Copy link

Right now, Mocha deems a Suite to be a root if the title for it is empty, as can be seen here:

function Suite (title, parentContext) {
  if (!utils.isString(title)) {
    throw new Error('Suite `title` should be a "string" but "' + typeof title + '" was given instead.');
  }
  this.title = title;
  // skipping non-pertinent stuff...
  this.root = !title;

Mocha creates a first Suite with '' for title, which marks that suite as the root suite. This is a problem because, it seems to me when the user uses an empty string to name a suite, this should not result in Mocha building a structurally deficient tree of tests. (A tree with two "root" suites.)

Being able to name suites with the empty string (without at the same time marking them as "root") is useful. Sometimes I have suites where testing what is logically a single piece of functionality is best done by dividing the tests into two groups: simple tests that can be performed with super fast setup code, more complex tests that need a costlier setup. Oh, I could use the setup code for the 2nd group for all tests, but that would increase the total run time. I end up with something like:

// Testing the method foo on class Bar.
describe("#foo", () => {
  describe("", () => { 
    beforeEach(() => {
       // create a trivial data structure that is quick to create and sufficient for the tests in this group.
    });

    // tests...
  });

  describe("", () => { 
    beforeEach(() => {
       // create a complex structure that is costlier to create but needed for these tests.
    }); 

    // tests...
  });
});

The fact that the suite uses different setups for the two groups of tests is an internal detail that is not useful to know in test reports. The need for different test setups does not always correlate with divisions that are meaningful from the point of view of reporting successes or failures. It often does, but not always.

So I would suggest that the code be changed to determine the value of root through some other test than the length of the suite's title. There could be a unique object that serves as a marker to indicate "this suite I'm building is a root suite". It could be "statically" added to Suite (e.g. Suite.Root so doing new Suite(Suite.Root, ...) would result in a root suite).

Ultimately, though, the root property seems redundant to me: a Suite which has no parent is a root suite, no? So "root"-ness should correlate with the absence of a set parent. But maybe there's some scenario I'm missing? At any rate, removing root cannot be done without breakage. The karma-mocha plugin, for instance, relies on it to produce reports (which is how I discovered the problem).

@drazisil drazisil added the type: feature enhancement proposal label Mar 30, 2017
lddubeau added a commit to lddubeau/karma-mocha that referenced this issue Oct 5, 2017
The root property is problematic because Mocha sets it true if the
description of a suite is the empty string. However, Mocha runs
perfectly well with suites that have empty names. See:

mochajs/mocha#2755

Instead of relying on the root property, rely on whether the parent
property is set. If not, then we are at the root.
@boneskull boneskull added the area: usability concerning user experience or interface label Dec 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: usability concerning user experience or interface type: feature enhancement proposal
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants