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

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

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

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

DinisCruz opened this issue Dec 23, 2014 · 38 comments
Labels
type: bug a defect, confirmed by a maintainer
Milestone

Comments

@DinisCruz
Copy link

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
Copy link
Contributor

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

@DinisCruz
Copy link
Author

different files

@dasilvacontin
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member

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.

@boneskull boneskull added type: bug a defect, confirmed by a maintainer status: accepting prs Mocha can use your help with this one! labels Dec 24, 2014
@dasilvacontin
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member

@dasilvacontin I'm behind that.

@dasilvacontin
Copy link
Contributor

@boneskull 💣 💥

@DinisCruz
Copy link
Author

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
Copy link

@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
Copy link
Contributor

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
Copy link

@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
Copy link

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
Copy link

Try that.

@twolfson
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
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
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
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
Copy link

@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
Copy link

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
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
Copy link
Contributor

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
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 pushed a commit that referenced this issue Jul 3, 2016
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
Copy link
Member

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

@boneskull boneskull mentioned this issue Jul 3, 2016
4 tasks
boneskull pushed a commit that referenced this issue Aug 1, 2016
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 status: accepting prs Mocha can use your help with this one! label Oct 10, 2016
@krmannix
Copy link

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
Copy link
Contributor

--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
Labels
type: bug a defect, confirmed by a maintainer
Projects
None yet
Development

Successfully merging a pull request may close this issue.