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

describe.only matches more that one describe test suite #1481

Closed
DinisCruz opened this Issue Dec 23, 2014 · 38 comments

Comments

Projects
None yet
@DinisCruz
Copy link

DinisCruz commented Dec 23, 2014

If I have these two test suites (just showing the first one)

describe 'Boolean',->

  it 'is_True',->
    expect(true.is_True    ).to.be.an('Function')
    expect(false.is_True   ).to.be.an('Function')
    expect(true.is_True()  ).to.be.true
    expect(false.is_True() ).to.be.false
    expect((1==1).is_True()).to.be.true
    expect((1==2).is_True()).to.be.false
    expect((1!=1).is_True()).to.be.false

and

describe 'Assert | Boolean',->
  it 'assert_Is_True', ->
    true.assert_Is_True.assert_Is_Function()
    true.assert_Is_True().assert_Is_True()
    (-> false.assert_Is_True()).assert_Throws()

If I add .only to the first one :

describe.only 'Boolean',->

both will now be executed:

image

image

and I would expect only the first one to execute

I assuming that mocha behind the scenes is not keeping a pointer to the actual tests to execute, it is just keeping the name/string, which is then searched on all tests/describes loaded (is that correct?)

@dasilvacontin

This comment has been minimized.

Copy link
Contributor

dasilvacontin commented Dec 23, 2014

Are the two suites in the same file or in different files?

@DinisCruz

This comment has been minimized.

Copy link
Author

DinisCruz commented Dec 23, 2014

different files

@dasilvacontin

This comment has been minimized.

Copy link
Contributor

dasilvacontin commented Dec 23, 2014

describe.only current only works per file. Not sure if it's intended design or a bug.

If you had tons of test files, making only work across all files would mean having to go through all of them to see if any of them tries to register a suite or a test-case using only. And with delayed suite creation, that could mean quite a lot of time.

What about using --grep?

@DinisCruz

This comment has been minimized.

Copy link
Author

DinisCruz commented Dec 23, 2014

my workflow is actually to have all tests in a runnable state and then only add the .only for the method or test suite I want to code (which makes it very efficient)

And it is easy to spot if there is an .only left behind because not all tests will run

@dasilvacontin

This comment has been minimized.

Copy link
Contributor

dasilvacontin commented Dec 23, 2014

What we could do is:
• go over each file. For each file, register suite/tests, check for only, destroy. (probably some people cant afford having all tests in memory at the same time, and we can use threads for checking the files)
• apply the only grep to all files, and proceed as normal

@DinisCruz

This comment has been minimized.

Copy link
Author

DinisCruz commented Dec 23, 2014

While you are doing that, another behaviour that happens which is breaks the test workflow is the fact that the .only on the describe overrides the .only on the it

Ideally the It should 'win over', in fact maybe even capture 'all its' that are flagged and only run these. This will be the equivalent of 'just run theses tests'.

My workflow is usually:

  1. have WebStorm to run tests on all changes (with a 2 sec delay)
  2. add an .only to an 'it' (i.e. test) that I want to work on
  3. code that test, in a really nice environment with just about realtime code execution
  4. when I'm done with my test, I usually want to work on another test, BUT I can't have the .only on the describe, so as soon as take the .only from the test I'm working on, I have 2 seconds to add the .only to another test of the describe (before the full test execution runs)

Does that make sense?

@dasilvacontin

This comment has been minimized.

Copy link
Contributor

dasilvacontin commented Dec 23, 2014

Yeah, the inner it should win over the describe.

What happens if you don't add .only in 2 seconds?

Wouldn't it be easier to add [wip] to the title and use --grep? Or use the new tag API? (if that made it into the v2.1 already)

@DinisCruz

This comment has been minimized.

Copy link
Author

DinisCruz commented Dec 24, 2014

well the whole test suite starts to runs which is not a massive issue since it is quick (about 15secs (including on UI driven tests, like the ones you can see here), and I can also stop it. But it would be nicer to not have that triggered.

The reason I don't like the --grep option is that I prefer not to change the full test execution config (on WebStorm and on package.config)

Regarding your comment on:

Wouldn't it be easier to add [wip] to the title and use --grep? Or use the new tag API? (if that made it
into the v2.1 already)

I'm not sure if I am fully aware of all those options and capabilities. Can you point me to a doc/article/blog about it?

Thanks

@dasilvacontin

This comment has been minimized.

Copy link
Contributor

dasilvacontin commented Dec 24, 2014

Okay, sorry about the tags thing, the PR hasn't been finished yet.

Can't you make the tests run only when you save the file?

We'll discuss .only behaviour, thanks for submitting the issue!

@boneskull

This comment has been minimized.

Copy link
Member

boneskull commented Dec 24, 2014

The reason I don't like the --grep option is that I prefer not to change the full test execution config (on WebStorm and on package.config)

If --grep is a valid workaround, then that's the solution you should go with for now.

The forthcoming tagging API will help you "tag" specs and suites so you can more efficiently run a subset of tests.

@boneskull

This comment has been minimized.

Copy link
Member

boneskull commented Dec 24, 2014

FWIW I don't really like the .only behavior as it's something of a misnomer. I think most people would prefer it to run one thing and one thing only.

@dasilvacontin

This comment has been minimized.

Copy link
Contributor

dasilvacontin commented Dec 24, 2014

I think most people would prefer it to run one thing and one thing only.

That's what makes sense. I think more than a few people must have thought 'wth' when they realised current behaviour.

@boneskull

This comment has been minimized.

Copy link
Member

boneskull commented Dec 24, 2014

I'm going to tag this as Bug. I can't think of an efficient, elegant solution off of the top of my head, so I'd be willing to look at PRs for this.

@dasilvacontin

This comment has been minimized.

Copy link
Contributor

dasilvacontin commented Dec 24, 2014

If --grep is a valid workaround, then that's the solution you should go with for now.

He could even run a different command depending on a ENV variable.

@boneskull

This comment has been minimized.

Copy link
Member

boneskull commented Dec 24, 2014

He could even run a different command depending on a ENV variable.

And many of the issues I see raised here are non-issues for those of us that choose to use a task runner (Grunt, Gulp, make, etc.). It's not like WebStorm restricts you to one run configuration, and one run configuration only. Guess it's just worth mentioning.

@dasilvacontin

This comment has been minimized.

Copy link
Contributor

dasilvacontin commented Dec 24, 2014

And many of the issues I see raised here are non-issues for those of us that choose to use a task runner (Grunt, Gulp, make, etc.).

I heard TJ wanted to kill --watch since it could be provided by a taskrunner or a simple watcher.

@boneskull

This comment has been minimized.

Copy link
Member

boneskull commented Dec 24, 2014

@dasilvacontin I'm behind that.

@dasilvacontin

This comment has been minimized.

Copy link
Contributor

dasilvacontin commented Dec 24, 2014

@boneskull 💣 💥

@DinisCruz

This comment has been minimized.

Copy link
Author

DinisCruz commented Dec 24, 2014

Hi @boneskull the issue is more for the ones that what to have semi-real time feedback on the tests executed. I already have a set-up where I can run all tests on background (ala Grunt, Gulp, make) , but what I really want to to have a REPL style interactive TDD environment, where I am continuously modifying the test(s) to execute (based on the tests I'm writing or code I'm modifying).

Actually on this topic, another option would be to try to detect (like ncrunch does) which tests are affected by code changes (and only run those)

@twolfson

This comment has been minimized.

Copy link

twolfson commented Dec 25, 2014

@dasilvacontin .only works across multiple files. It leverages mocha.grep which is across the entire process.

mocha.grep(suite.fullTitle());

This is the same method that --grep uses from the command line:

this.grep(options.grep);

The issue that @DinisCruz is having is both test suite names use Boolean so it is matching both contexts. To prove this, I have created a gist:

https://gist.github.com/twolfson/9b769b716dd7bad383ac

When it is run normally, it runs only the suite from test-one.js:

> mocha test-one.js test-two.js

  one with a .only
    ✓ is run separately from other tests 


  1 passing (4ms)

When we use change the title in test-one.js so it will match against both, it runs both and fails (as expected):

// Updated `test-one.js`
describe.only('.only', function () {
  it('is run separately from other tests', function () {
> mocha test-one.js test-two.js

  .only
    ✓ is run separately from other tests 

  two without a .only
    1) is not run

  1 passing (5ms)
  1 failing

  1) two without a .only is not run:
     AssertionError: 1 === 2

@DinisCruz As a workaround until .only stops using grep, make it so the names won't match against each other. For example:

describe 'A Boolean constructor',->
describe 'An assertion against the Boolean constructor',->
@dasilvacontin

This comment has been minimized.

Copy link
Contributor

dasilvacontin commented Dec 25, 2014

Sorry, my bad.

We need a PR for this then. Not even with regex support for grep we could make it work for all cases.

@twolfson

This comment has been minimized.

Copy link

twolfson commented Dec 25, 2014

@boneskull Would it be possible to create a mocha.only which accepts a unique identifier for a context? For example, pass through the reference for the suite directly. However, in practicality that would be difficult to check parents against.

Maybe we can do something like test-file.js:describe[1]:it[2] where each time we descend into another context it stores what type it was an its index (e.g. describe and 1 = second describe block). That would still be able to be easily implemented as we can still leverage regexp matching..

@Tarabyte

This comment has been minimized.

Copy link

Tarabyte commented Dec 27, 2014

Current behaviour is really really unexpected.
It was breaking my workflow so I've used the following workaround (coffee)

describe.only = ((only) ->
  (args...) ->
    if typeof args[0] is 'string' then args[0] += '\u200b' #zero-width whitespace
    only.apply @, args
)(describe.only)

@tylerhjones tylerhjones referenced this issue Jan 7, 2015

Closed

.only patch #1492

@tylerhjones

This comment has been minimized.

Copy link

tylerhjones commented Jan 7, 2015

Try that.

@twolfson

This comment has been minimized.

Copy link

twolfson commented Jan 7, 2015

@tylerhjones That solution will fit most cases but not if there are 2 cases with the same describe block and I use a .only on one of them.

// We should only run this block
describe.only('A server', function () {
  describe('receiving an HTTP request', function () {
    it('replies successfully', function () {
    });
  });
});

describe('A server', function () { /* Similar setup but different test */ });

We need to solve it from file and position within the file. That is a valid unique identifer for each block.

@Vanuan

This comment has been minimized.

Copy link

Vanuan commented Feb 10, 2016

it works correctly, BTW:

it.only('name')
it('another name')
# only 'name' is executed

a8m added a commit to a8m/mocha that referenced this issue Mar 9, 2016

feat(runner): fix '.only()' exclusive feature, mochajs#1481
This PR fix mochajs#1481, and also extends the .only() behaviour.
(i.e: it's not use grep anymore, support suite, test-case or both, add
the ability to run multiple .only)

a8m added a commit to a8m/mocha that referenced this issue Mar 9, 2016

feat(runner): fix '.only()' exclusive feature, mochajs#1481
This PR fix mochajs#1481, and also extends the .only() behaviour.
(i.e: it's not use grep anymore, support suite, test-case or both, add
the ability to run multiple .only)
@RavenHursT

This comment has been minimized.

Copy link

RavenHursT commented Mar 18, 2016

@Vanuan lol.. I totally agree.. this has been marked as a bug since Dec 2014.

This bit me in the butt today as well. The .only behavior in Mocha is extremely confusing.

@deltreey

This comment has been minimized.

Copy link

deltreey commented Mar 23, 2016

wow, I thought I might help by putting in a fix, but the latest pull request changes 17 files and hundreds of lines of code...how complicated is this .only check?

@Vanuan

This comment has been minimized.

Copy link

Vanuan commented Mar 23, 2016

It's not complicated. It's that maintainers are trying to fix more and more issues in one pull request.

@danielstjules

This comment has been minimized.

Copy link
Contributor

danielstjules commented Mar 23, 2016

changes 17 files and hundreds of lines of code...how complicated is this .only check?

Those are mostly integration tests. The lib change itself is 6 files and 83 loc with comments.

@Vanuan

This comment has been minimized.

Copy link

Vanuan commented Mar 23, 2016

First pull request, #1492 covered all use cases except duplicate test names. For me, duplicate test names is a bug, not a feature. Mocha should've stack-traced if smb registered duplicate tests.

boneskull added a commit that referenced this issue Jul 3, 2016

feat(runner): fix '.only()' exclusive feature, #1481
This PR fix #1481, and also extends the .only() behaviour.
(i.e: it's not use grep anymore, support suite, test-case or both, add
the ability to run multiple .only)
@boneskull

This comment has been minimized.

Copy link
Member

boneskull commented Jul 3, 2016

PR for this is merged into v3.0.0 branch via 8a75434

@boneskull boneskull closed this Jul 3, 2016

@boneskull boneskull referenced this issue Jul 3, 2016

Closed

Release: v3.0.0 #2350

4 of 4 tasks complete

boneskull added a commit that referenced this issue Aug 1, 2016

feat(runner): fix '.only()' exclusive feature, #1481
This PR fix #1481, and also extends the .only() behaviour.
(i.e: it's not use grep anymore, support suite, test-case or both, add
the ability to run multiple .only)

@boneskull boneskull removed the help wanted label Oct 10, 2016

@krmannix

This comment has been minimized.

Copy link

krmannix commented May 15, 2017

Is it possible to use the old behavior (i.e., .only including all blocks, regardless of file, at the same level & same identifier) with a flag or something? I structured tests based on the old behavior, and it was nice to run all tests for a specific identifier with a single .only

@ScottFreeCode

This comment has been minimized.

Copy link
Contributor

ScottFreeCode commented May 17, 2017

--grep (or .grep in the programmatic API) is (and always was) equivalent to the old .only behavior. Or maybe it was equivalent to --fgrep/.fgrep; I forget which. Use whichever fits your use case. ;^)

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