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

Core: Implement only/skip and todo for modules #1168

Closed
wants to merge 2 commits into from

Conversation

Arkni
Copy link
Contributor

@Arkni Arkni commented Apr 22, 2017

This is a work in progress to implement QUnit.module.{only, skip, todo}.
I still need to add some tests and make sure everything is working as expected.

Suggestions are more than welcome.

/Cc @trentmwillis

@Arkni
Copy link
Contributor Author

Arkni commented Apr 23, 2017

About the tests, should I add a file for each of the methods or there is/are file(s) that I can extend with new unit tests ?
Ended up adding a new file as I thought the new tests are not related to any of the existing ones.

@Arkni Arkni changed the title [WIP] Core: Implement only/skip and todo for modules Core: Implement only/skip and todo for modules Apr 26, 2017
@Arkni
Copy link
Contributor Author

Arkni commented Apr 26, 2017

I've rebased this and it is now ready for review.

Copy link
Member

@trentmwillis trentmwillis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay in reviewing. This past month has been crazy.

FWIW, I think breaking this up into PRs for each function (skip, todo, only) would make it easier for us to review + merge more quickly.

config.currentModule = module;
}

function module( name, testEnvironment, executeNow ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a TODO comment here noting that we should extract this to a new file? (I think it'll require some refactoring, so I don't want it to be in this PR).

suiteReport: new SuiteReport( name, parentSuite )
suiteReport: new SuiteReport( name, parentSuite ),

// Pass along `skip` and `todo` properties from patent module, in case there
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: patent -> parent

return;
}

moduleStack.length = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should do this. If you focus a module that is nested within another, then the moduleStack would still need to be used.

@Arkni
Copy link
Contributor Author

Arkni commented May 22, 2017

No worries!

I will try to split this PR & address the above comments as soon as possible.

Thanks!

@Arkni
Copy link
Contributor Author

Arkni commented May 24, 2017

Hi @trentmwillis

I addressed your requested changes in #1179. The linked PR only contain the implementation of QUnit.module.only(). The other two functions will be implemented in subsequent PRs.

Closing this in favor of #1179

@Arkni Arkni closed this May 24, 2017
@Arkni Arkni deleted the module-only-skip-todo branch May 24, 2017 23:11
Arkni added a commit to Arkni/qunit that referenced this pull request Jun 15, 2018
Arkni added a commit to Arkni/qunit that referenced this pull request Jun 15, 2018
trentmwillis pushed a commit that referenced this pull request Jun 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants