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

Replace minitest.cr in favor of std-lib spec #334

Merged
merged 27 commits into from
Mar 23, 2020

Conversation

bcardiff
Copy link
Member

This pr removes minitest to keep dependencies within crystal-lang org.

Since the integration spec use data initialized once arranche the spec to be spec/unit and spen/integration.

  • The Makefile targets remains the same.
  • I left as comments the custome failure messages.
  • I left the helper assertion methods used in integration specs that have no matching in std-lib spec.
  • To make typing happier, I introduced run! to be the alternative of run that captures the output.
  • The MockResolver was dead code so it is removed.

Nothing against minitest.cr (sorry @ysbaddaden!) but is important to keep dependencies within the org.

@RX14 RX14 requested a review from ysbaddaden March 21, 2020 13:04
Copy link
Contributor

@ysbaddaden ysbaddaden left a comment

Choose a reason for hiding this comment

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

Fine by me.

Thought, I think run! is confusing and couldn't be less explicit; keeping custom assertions and refutations is kinda weird along should expectations; it also leaks a bunch of methods into the global space... I guess spec ain't as good as minitest 😏

@bcardiff
Copy link
Member Author

Yeah, there some advantage of minitest in some scenarios.

Another thing I miss in the current spec that is present in rspec is the expect(expr).to ... because it gives a cleaner prefix of an assertion. Something that minitest have.


Was there a reason to not capture all the output in run commands or was more an evolution of the code? Because we would make the output to always be captured.

@ysbaddaden
Copy link
Contributor

Yes, we should just always capture, it shouldn't be noticeably slower.

Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

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

I love it! This not only removes non-org dependencies it also removes friction and makes it easier for contributers when you don't have to learn another spec framework.

For some reason the commits seem to be out of order. The "Migrate ..." commits should probably be the first ones but they're somewhere in the middle...

spec/versions_spec.cr Outdated Show resolved Hide resolved
spec/integration/spec_helper.cr Outdated Show resolved Hide resolved
spec/integration/spec_helper.cr Show resolved Hide resolved
File.join(application_path, "lib", project, *path_names)
end

def debug(command)
Copy link
Member

Choose a reason for hiding this comment

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

What is this used for?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess it's a helper method to debug failure on commands. It was there I didn't want to disrupt anybody's workflow.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep. Useful when an integration test starts failing: add a debug run instead of trying to reproduce the test case or printing the output manually.

spec/support/cli.cr Outdated Show resolved Hide resolved
@bcardiff
Copy link
Member Author

@straight-shoota some feedback addressed. I didn't want to change/refactor the specs a lot. I think that can be handled later. The goal is to drop the non org dependency and allow compilation on master easily.

Improvements in the specs can come later IMO.

We need to fix build on master and promptly tag a molinillo / shards release so Crystal nightlies can build again.

@bcardiff
Copy link
Member Author

Regarding the commit history, I've done some rebases along the way and that bothers github ui. But we can squash all these commits if preferred. I commit it that way in case other PRs were merged in between so a rebase would be easier for me.

@jkthorne
Copy link

I still would like to see a test unit style added to the std-lib. I think it is a little weird that a project under the Crystal Org cannot have a test unit style.

@asterite
Copy link
Member

I agree. We should eventually enhance spec to provide test-unit like tests. This is not only a cosmetic matter: with test-unit you can define classes, inheritance, initializers (so better setup), share variables, etc.

But we can do it later, even after 1.0

@jkthorne
Copy link

@asterite I am sure more people have asked but I made a post about it a while ago.
crystal-lang/crystal#7042

@asterite
Copy link
Member

On second though, there's minitest.cr which can be used by any project. I guess it's find that crystal-lang org projects only use specs. That way any core contributor only needs to know spec.

@straight-shoota
Copy link
Member

This is definitely not the topic of this discussion.

@bcardiff
Copy link
Member Author

@straight-shoota are we good on your side to merge this as is?

@ysbaddaden
Copy link
Contributor

Yes, please squash to keep history nice. Also, as @RX14 pointed out, there seems to be Spec.before_all after all.

@bcardiff bcardiff merged commit 521ce3b into crystal-lang:master Mar 23, 2020
@bcardiff bcardiff deleted the drop-minitest branch March 23, 2020 17:55
@Sija Sija mentioned this pull request Mar 24, 2020
@bcardiff bcardiff added this to the v0.10.0 milestone Mar 27, 2020
taylor pushed a commit to vulk/shards that referenced this pull request Aug 11, 2020
f-fr pushed a commit to f-fr/shards that referenced this pull request Jan 2, 2021
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.

None yet

6 participants