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

[WIP] Add specs for spec output #5671

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

Conversation

straight-shoota
Copy link
Member

This a work in progress trying to add specs for spec output.

Until know, there are no specs for this at all. Unfortunately the spec implementation is not very modular, so I had to mock it quite extensively to be able to run it in a spec context. But it work and required only a minimal change in the actual implementation.

It's far from ready but I'd like to get some feedback on this approach.

This could be useful for implementing #5633

@RX14
Copy link
Contributor

RX14 commented Feb 2, 2018

Honestly i'd advocate for invoking bin/crystal and on some example spec files and observing the output over all this stubbing.

@luislavena
Copy link
Contributor

Honestly i'd advocate for invoking bin/crystal and on some example spec files and observing the output over all this stubbing.

On separate issue, @asterite has commented that he would prefer programmatically initializing/using code than shelling out externally. That is also good considering shelling out might be expensive on other platforms (ie. Windows).

IMO the excessive stubbing is caused by the lack of internal API on Spec, in which case can be resolved in future PRs.

@straight-shoota
Copy link
Member Author

It would certainly make sense to have some integration test running bin/crystal spec - to ensure it really works as expected.
But It think programmatic testing is preferable in general to validate specific behaviour. And I guess this could easily be improved by refactoring some tight couplings out of Spec.

@jhass
Copy link
Member

jhass commented Jun 7, 2019

Where do we want to go with this? I think given what this tries to cover, the amount of added complexity is actually pretty fair. So I would be willing to merge this in as a basis. Then if somebody has the idea and time to reduce the stubbing or find a better approach in general they can update this.

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.

None yet

5 participants