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

Exposing skip for evaluation by onsi #382

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Exposing skip for evaluation by onsi #382

wants to merge 1 commit into from

Conversation

gaffo
Copy link

@gaffo gaffo commented Oct 12, 2017

Opening for dicussion for fixing #377

@onsi
Copy link
Owner

onsi commented Oct 13, 2017

@gaffo this seems like a reasonable enough approach to me. Yeah, it's clunky, but it's also explicit and relatively simple so I'm in favor. Wanna add some tests and some documentation (under the gh-pages branch)?

@gaffo
Copy link
Author

gaffo commented Oct 13, 2017 via email

@onsi
Copy link
Owner

onsi commented Oct 13, 2017

i’d suggest doing it as an integration test - you should be able to follow the patterns in the /integration folder.

@gaffo
Copy link
Author

gaffo commented Oct 25, 2017

Any specific file I should dump it in in integration, next to something else? do I need to check the error text is correct or just make sure it compiles and runs correctly?

@onsi
Copy link
Owner

onsi commented Oct 26, 2017

Actually - now that I think of it DescribeSkip might be confusing since there is a Skip already. Could we mirror Gomega and use DescribeWithOffset etc.?

I think you could then create a new integration/offset_test.go. And yeah I think we'd want to make sure the error text is correct to catch regressions. An easy way to do that is make a new fixture, run the test, grab the line number, then just hard code that in the test. You should be able to pattern match and copy/paste your way to victory with the integration tests but let me know if I can help.

@gaffo
Copy link
Author

gaffo commented Oct 26, 2017 via email

@williammartin
Copy link
Collaborator

@gaffo Looks like maybe this has been languishing, do you think you'll get a chance to implement Onsi's suggestions?

@gaffo
Copy link
Author

gaffo commented Dec 5, 2017 via email

@pmzajaczkowski
Copy link

It looks like all of the conditions are met to merge-in this pull request. Could you accept it? :)

@williammartin
Copy link
Collaborator

@pmzajaczkowski Changes have been requested earlier in the comment chain. If you'd like to tackle those and open a separate PR I'd be happy to accept that.

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

Successfully merging this pull request may close these issues.

4 participants