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

Makefile: refactor test recipe #12979

Merged

Conversation

straight-shoota
Copy link
Member

The spec recipe builds spec/all_spec.cr which combines std_spec, compiler_spec and primitives_spec into a single spec suite. This is huge and requires a lot of resources, both for building as well as running the specs. It can easily exhaust many build system's memory capabilities.

I don't think we're using it anywhere whatsoever. And it's really not recommended due to the intense resource requirements.

But since it's there and mentioned as the top test recipe, users (especially package maintainers) are likely to pick it up.

This patch includes the following changes:

  • Rename spec to all_spec and add a note that it's expensive.
  • Add test as a replacement which runs the parts (std_spec, compiler_spec and primitives_spec) individually.
  • Update the example make commands

Copy link
Contributor

@HertzDevil HertzDevil left a comment

Choose a reason for hiding this comment

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

Makefile.win needs the same changes

Makefile Outdated Show resolved Hide resolved
@asterite
Copy link
Member

Why the name test? It's a bit confusing given that tests in Crystal are called specs.

By the way, I used to use make spec all the time to make sure all specs pass before I submit a PR, but I think it's okay to rename this to all_spec. But maybe test should be spec?

@straight-shoota
Copy link
Member Author

@asterite The idea for renaming is to make clear that it does something different than make spec. The purpose is still the same of course, and maybe that's good enough, but the mode is different.

In any case, test is a more general term and by convention a typical standard target for Makefiles.

@asterite
Copy link
Member

The idea for renaming is to make clear that it does something different than make spec

Oh, then I'm confused. What does make spec do now with this PR?

@straight-shoota
Copy link
Member Author

The spec target doesn't exist anymore.
I was expecting make spec to fail, but it doesn't. The spec folder exists and make recognizes that. It just reports there's nothing to do.

Then it's probably better to define spec as a phony target. It could either do what test does (implicitly change the meaning of make spec to run specs individually) or we could let it fail with an error message to use either make test or make all_spec instead.

@asterite
Copy link
Member

Add test as a replacement which runs the parts (std_spec, compiler_spec and primitives_spec) individually.

I don't understand why we can't change spec to do that instead. Why rename spec to test?

@straight-shoota
Copy link
Member Author

Like I said in #12979 (comment) the idea was to make make spec fail so you have to decide whether you really want the all_spec behaviour with a single executable (then you'd need to make it explicit). If not, change it to test instead.

@asterite
Copy link
Member

What would be the dangers of naming it spec? Let's consider a few users:

  1. Me, or others wanting to check all specs pass before pushing code: it works like before, except that it consumes less resources, and it might be even faster because we don't need to wait for all specs to be compiled (say: the first batch fails, we get feedback earlier)
  2. Package maintainers: they probably check the error code to see if all specs pass. Nothing changes for them.
  3. Others? I'm not sure.

With this change, if some packages relied on spec you are now forcing everyone to update their code or flow... for no real reason.

@straight-shoota
Copy link
Member Author

Okay, let's keep the name spec.

I'll still keep test as an alias because that's the makefile convention.

HertzDevil
HertzDevil previously approved these changes Jan 19, 2023
@HertzDevil HertzDevil self-requested a review January 19, 2023 21:21
Makefile Outdated Show resolved Hide resolved
@straight-shoota straight-shoota added this to the 1.8.0 milestone Jan 19, 2023
@straight-shoota straight-shoota merged commit 0ebef4f into crystal-lang:master Jan 20, 2023
@straight-shoota straight-shoota deleted the refactor/makefile-spec branch January 20, 2023 23:22
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.

3 participants