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

.only patch #1492

Closed
wants to merge 2 commits into from
Closed

.only patch #1492

wants to merge 2 commits into from

Conversation

tylerhjones
Copy link

patch for describe . only in response to #1481

patch for describe . only
@dasilvacontin
Copy link
Contributor

I have to test this patch.

@dasilvacontin dasilvacontin changed the title Update mocha.js .only patch Feb 2, 2015
@bjornstar
Copy link
Contributor

LGTM, I think that it's a good idea to make describe.only match the behavior of it.only.

@dasilvacontin
Copy link
Contributor

This patch doesn't work (yay our tests). You are only patching one of the interfaces and you are doing it in the compiled mocha file. The compiled mocha file is auto-generated from the lib for the releases.

You have to patch the following files:

  • lib/interfaces/bdd.js
  • lib/interfaces/qunit.js
  • lib/interfaces/tdd.js

@dasilvacontin
Copy link
Contributor

ping @tylerhjones

@tylerhjones
Copy link
Author

thanks for the info, ill work on it later tonight MST

@dasilvacontin
Copy link
Contributor

Cool! I was just pinging in case you missed my message.

@tylerhjones
Copy link
Author

This currently passes npm test, but I have not yet added a separate test to assert that this .only bug does not appear again.

I was attempting to add the test to test/runner.js but am not sure if that is the correct location or if I am approaching the test correctly.

@dasilvacontin
Copy link
Contributor

The mocha.js file at the root of the repo shouldn't be modified; it is compiled from the source whenever a release is prepared.

Regarding test location, I should suggest adding tests for each of the interfaces at test/acceptance/misc/only.

Please squash your commits into a single one after the changes. Thanks!

@danielstjules
Copy link
Contributor

Thanks for the PR! For issue #1481, it looks like the team has decided to go with #1591 which fixes some related only bugs due to the use of RegExps. Appreciate you taking the time to work on this though!

@Vanuan
Copy link

Vanuan commented Feb 10, 2016

I don't think that fixing multiple issues with one pull request is a good idea.
This is a good PR, it's just missing some tests.

@Vanuan
Copy link

Vanuan commented Feb 10, 2016

#1591 is closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants