[RFC] What's the future of Spec? #8289
Comments
|
Maybe we can iterate some ideas that do not match 100% with rspec. With implicit or explicit creation of the context of execution. private class Context
property something = SomeObj.new
def {before|after|around}_{suite|each}
end
end
describe "something", using: ctx = Context.new do
it "..." do
use ctx.something
end
it "..." do
use ctx.something
end
endThe feature I am most interested in is tagging the specs. |
|
At that point it wouldn't it be less complex to just do like def do_with_context
context = Context.new
yield context
end
do_with_context do |ctx|
describe "something" do
it "..." do
use ctx.something
end
it "..." do
use ctx.something
end
end
endWhich worked before. It's what I was using for Athena tests. Method starts a server in the background with the given config/env settings, news up a http client pointing to that server, yields the client and executes the tests, then closes the server. Including a feature like that would just be moving complexity from the user's code into the compiler IMO. Keeping things simple would be more preferable in my opinion. |
|
@bcardiff the problem is there's no way to implement that with the current language. 👍 to revert spec to the previous form? |
|
@asterite It's too early in my opinion. |
|
Let's remember that Ruby's Minitest has What's missing is a context to share state for each before/it/after to run in. In Minitest (and maybe RSpec) there is an instanciated class so we have a |
|
Maybe it's just time to give up on the DSL and just go the class MyTest < UnitTest
def before_all
@shared = create_shared
end
def test_a
assert_sane @shared
end
def test_b
assert_happy @shared
end
endAs much as I'd hate that... Now of course we could have the macro foo create classes like that, but that goes against Ary's KISS wishes and also if we then allow nesting that easily creates a shitload of classes overloading the compiler, as early experiments by some of us (mine's at https://gist.github.com/jhass/e9219dd9da24204a073d) have shown in the past. |
|
Thank you all for some good answers! I'm also surprised my proposal got 3 👍 but 5 👎 so far, which means maybe the current approach isn't that bad. I'm starting to think that if before the spec refactor doing setup/teardown was already kind of hard, now it's harder, but that might be a good thing. For example in this Haskell article they mention that to do setup/teardown you have to do it inside each test, which makes it clear what goes on (you don't need to look up up in the file to see whether there's some set up going on), so each test becomes self-contained. This is something I really like about our spec, and with this change we encourage this more (you can't have a test share a variable declared outside of it). I do think adding Finally, having a form of controlled I'll first send a PR for the hooks, then I'll draft a PR for |
|
I love having the focus feature, and have been using it every day since it came out. I also love having the I agree that it's nicer to have more simple code, but if the language is simple, then each project becomes more complex. I also believe that even though our Spec takes most of it's DSL from RSpec, I think it's ok if we break off and make it more "crystal-like". We could pass in context to the blocks. Doing describe SomeClass do |ctx|
ctx.before_each do
only_run_before_each_thing_in_ctx
end
describe "sub thing" do |ctx2|
ctx2.before_each do
another_block
end
end
endNot to say it needs to be exactly like that, but the idea being that we don't have to copy rspec 100%. We just need what's best for our community to make testing a first class citizen, and fun to do while being robust. edit Since I had to sell my team on why I believe keeping specs on the path it is now is the way to go. It's my understanding that with the new spec path, we can have these:
|
|
@asterite I like your point here:
I think this is super important. RSpec specs with tons of befores can get unwieldy. With that said, sometimes So I would not be against adding those features. I think the real problem is using If you have a lot of setup I think private methods work super well, but I realize this may also be my opinion. Some people love |
|
I've been using my own patch that adds Regarding spec ordering, in my opinion specs should be randomly sorted by default, with the option to specify alternate orderings. There are so many errors that can go unnoticed if you always run specs in the same order. Depending on specs to be run in some particular order is an anti-pattern, and preventing it entirely would be as big a win in my book as how the compiler prevents A key piece of the random ordering feature, though, should be an option to re-run with the exact previous ordering upon failure. So upon failure, we print out "you can run I realize this creates the issue rspec has to deal with where setup code is difficult because I agree that using instance variables in hooks is an anti-pattern, so other than adding a few more hook types like |
|
This is my feature wishlist for
|
|
@sam0x17 When using Regarding your wish list:
|
|
Parallel specs by default would, on the other hand, be a great way to actually find real issues early, for the same reason random ordering do. It may force some additional ways to flag stuff that uses the same external resource though. |
|
I could see implementing |
|
@straight-shoota that's some very good feedback, and generally I agree. Having seen a lot of rails apps in production at various companies, I will say this. Once an app is large enough, there are almost always going to be "unstable" specs, and usually managing these becomes a CI nightmare. Are they bad? yes. Are they terrible? yes. This is a very common thing with integration testing involving javascript. Sometimes it just chaotically doesn't work. So my motivation for wanting a depends_on style specs is probably going to end up being my own shard when I get around to doing it. I believe in a testing methodology where coverage is detectable (imagine doing this with annotations) and we can completely determine which specs need to run for a particular set of changes or to cover a particular piece of code. Didn't know about pending -- that does what I need just fine |
|
@straight-shoota to illustrate my point based on your comment,
Once you get to 3000+ specs (which is the average spec suite size of companies I have worked with), occasional almost always happens at that volume, unless there are zero integrations specs. |
|
Some of the mentioned features are already included in feature rich shards like Spectator (which integrates modern RSpec features and syntax). Is there so far a general consensus on the 'high-level general idea' of Spec (minimal vs feature rich)? |
|
I think in general we are trying to keep things simple, but still have goodies like parallelism, around_each, grouping, etc., without complicating the existing API footprint.
In other words, be a better, simpler, faster rspec.
… On Oct 14, 2019, at 5:37 AM, Alexander ADAM ***@***.***> wrote:
Some of the mentioned features are already included in feature rich shards like Spectator <https://gitlab.com/arctic-fox/spectator>.
So far I thought Spec of the standard library just was focused to be kind of minimal but yet functional. On the other hand it would obviously be nice having wonderful spec features available within the standard library as well.
Is there so far a general consensus on the 'high-level general idea' of Spec (minimal vs feature rich)?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#8289?email_source=notifications&email_token=AAOE4LL6NV65NI7ZUML46FDQOQ4WDA5CNFSM4I6JVVS2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBD5QYI#issuecomment-541579361>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAOE4LNRXVYR3BSOH3G42XLQOQ4WDANCNFSM4I6JVVSQ>.
|
|
Yes, stdlib spec should be minimal but include enough features for a basic experience. Additional features can be added by third party shards. |
|
I prefer some of the features of spec and testunit. I like the assert style tests. I find it to be more predictable in my assumption of what is going on. I know some people find the syntax adds clarity, but I like the simplicity of the interface. I also like the structures of When it comes to structure, I like to have more helper methods call in specs and fewer hooks surrounding test suites. I find there are a host of problems that can come up, and they are not offset by the consciousness or line count of the code. I want an assert style syntax with describing and it blocks and less reliance on before and after hooks. |
|
I agree on "the fewer hooks, better it is". |
This was already answered by @wontruefree:
And I fully agree to that. Until recently, there have not been any useful hooks in That being said, now that hooks have been implemented (with the updated internals this wasn't a big deal), I don't mean for them to go away. I just won't use them much (or at all). And I'll probably advocate to stay with helper methods in every project I'm contributing ;) |
|
Note, though, that the stdlib and compiler don’t involve a database (production apps typically do), and with many of the database shards (clear-orm for example), the ONLY way to put something in a transaction is wrapping via a block. This means that without `around_each` hooks, it is physically impossible to wrap specs in database transactions without patching the ORM itself or doing it manually in every single spec.
In general, we should be careful not to bias our `Spec` feature set based on the admittedly atypical situation of writing specs for a standard library and compiler. Real-world apps have very different needs, and much grittier, more fallible abstractions, tendencies, and runtime behaviors. Spec should be useful and usable for everyday app development, not just compiler development.
|
|
just to clarify I think hooks are useful and handy to have in a test suite. But like in the words of the agile manifesto I prefer setup helpers over hooks. |
|
These are the sorts of things I regularly have to put in hooks:
Things I never use hooks for:
|
|
@sam0x17 Yes, I've written lots of database specs as well. They're usually wrapped in Of course, hooks can be useful at times and I won't condemn using them. |
|
Right now using my `around_each` patch.
… On Oct 14, 2019, at 2:00 PM, Johannes Müller ***@***.***> wrote:
@sam0x17 <https://github.com/sam0x17> Yes, I've written lots of database specs as well. They're usually wrapped in transaction blocks. It's not a big deal. It's just nice to work with. It makes the connection directly available as block arg. How do you get access to that when it's initialized in a before_each hook?
Of course, hooks can be useful at times and I won't condemn using them.
And I think we all agree that hook support in spec is here to stay.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#8289?email_source=notifications&email_token=AAOE4LMUEB6ICRTQ5V2LHB3QOSXURA5CNFSM4I6JVVS2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBF2FAI#issuecomment-541827713>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAOE4LKMTLLLH276QVTGZHDQOSXURANCNFSM4I6JVVSQ>.
|
|
I personally think that Crystal's Spec should offer the following, but not much more:
As most of these are already implemented or in PRs, I think the direction it's going is good, though I do understand that the current implementation may make it difficult to do particular things like the variable sharing. Specifically on the assert style vs should style, I personally don't like the assert style, preferring the should/expect style. However, I think that ultimately comes down to how each person's brain prefers to see things... infix notation (a eq b) vs prefix notation (eq a b). I think it would be good in this particular instance for Crystal to offer both to accommodate both types of person.
I've always felt this was one of the downsides to the RSpec style...that is, a lot of setup ends up "far away" from the code under test. But, this happens with helper methods too...you create a method at the top of the file and then reference it over an over at the bottom of the file, so I'm not sure it's something that can be avoided. I agree with @paulcsmith that where it gets extremely unwieldy is when you have |
|
@Fryguy Regarding In terms of what we should do, I have no problem with adding |
|
@sam0x17 How do you get access to the prefabs? |
|
@asterite I use a macro that makes them available via method names at the top level (basically cheatsy global variables), and I also have |
I think databases are kind of unique in that respect because they offer transactional rollbacks, which can even support parallelism. If you're writing specs, that, say, shell out to commands that modify the system, then I think it's a bit harder. I see |
|
@Fryguy that's true. I never thought of it that way. If I was testing stuff like that I would default to using helper methods, but it's true one could use |
|
high level description of how my prefab setup works: I have a I could clean this up and make it into a shard if people are interested. It's fairly ORM agnostic, though it requires a |
|
i have a simple example that works for me require "spec"
require "./../../game/Round.cr"
require "./../../models/Character.cr"
private class Context
getter :enemy
def initialize()
@luck = 10
@hero = Models::Character.new("user1", 100.0, 30.to_i64)
@enemy = Models::Character.new("user2", 100.0, 30.to_i64)
end
def enemy_energy_after_calling_update_state()
_, enemy_after_round = Game::Round.new.update_state(@hero, @enemy, @luck)
enemy_after_round.energy
end
end
describe Game::Round do
describe "#update_state" do
context "when luck factor is less than 15" do
ctx = Context.new
it "returns enemy with the same energy before round" do
ctx.enemy_energy_after_calling_update_state.should eq ctx.enemy.energy
end
end
end
end |
|
From a reusability standpoint I kinda like #8289 (comment) this approach. The main reason being since they are just classes you are able to use inheritance/modules to share common logic between tests. Such as by using inheritance, modules, and/or abstract methods to more easily setup specs for a common thing. I ran into this recently when working on my validation shard. It would be nice to have something like: abstract struct AbstractComparisonTestCase < Crystal::TestCase
protected abstract def create_constraint(**args)
protected abstract def create_validator
protected abstract def valid_comparisons
protected abstract def ivalid_comparisons
def test_valid_comparisons : Nil
self.valid_comparisons.each do |(expected, actual)|
assert_no_violation create_validator, create_constraint(value: expected), actual
end
end
def test_invalid_comparisons : Nil
self.ivalid_comparisons.each do |(expected, actual)|
assert_violations create_validator, create_constraint(value: expected), actual
end
end
end
struct EqualToValidatorTest < AbstractComparisonTestCase
protected def create_validator
EqualToValidator.new
end
protected def create_constraint(**args)
EqualTo.new **args
end
protected def valid_comparisons
[
{1, 1},
{"foo", "foo"},
{ComparableMock.new(5), ComparableMock.new(5)},
{Time.utc(2020, 4, 7), Time.utc(2020, 4, 7)},
{1, nil, "nil"},
]
end
protected def ivalid_comparisons
[
{2, 1},
{"bar", "foo"},
{ComparableMock.new(10), ComparableMock.new(5)},
{Time.utc(2020, 4, 8), Time.utc(2020, 4, 7)},
]
end
endMy current workaround for this would be to just use private method/constants and a macro to define the describe blocks: private def create_validator
AVD::Constraints::EqualToValidator.new
end
private def create_constraint(**named_args)
AVD::Constraints::EqualTo.new **named_args
end
private VALID_COMPARISONS = [
{1, 1, "numbers"},
{"foo", "foo", "strings"},
{ComparableMock.new(5), ComparableMock.new(5), "comparable types"},
{Time.utc(2020, 4, 7), Time.utc(2020, 4, 7), "times"},
{1, nil, "nil"},
]
private INVALID_COMPARISONS = [
{2, 1, "numbers"},
{"bar", "foo", "strings"},
{ComparableMock.new(10), ComparableMock.new(5), "comparable types"},
{Time.utc(2020, 4, 8), Time.utc(2020, 4, 7), "times"},
]
# This would live in spec_helper
macro define_comparison_test
describe "#validate" do
VALID_COMPARISONS.each do |(expected, actual, message)|
it "valid #{message}" do
assert_no_violation create_validator, create_constraint(value: expected), actual
end
end
INVALID_COMPARISONS.each do |(expected, actual, message)|
it "invalid #{message}" do
assert_violations create_validator, create_constraint(value: expected), actual
end
end
end
end
describe AVD::Constraints::EqualToValidator do
define_comparison_test
endId be curious to hear if anyone has a better idea |
|
@Blacksmoke16 Can you use minitest.cr instead> |
|
@asterite Probably. Although I'm hesitant about introducing an external dependency JUST for that. Plus it's just something else someone has to learn if they want to contribute. |
|
I think there's no need for |
|
Having the macro content be in every spec file would also work. My thought was that this way the code is centralized. I.e. if you wanted to add something else you update the macro and possibly add some new method to each file as opposed to updating the describe blocks and add a method for each file. As as far as I'm aware, a macro is the only way to reuse a describe block in multiple places. EDIT: I just realized I could probably just define it as a method and pass in the two comparison arrays as arguments. That would probably the better approach. EDIT2: Eh, then I would have to pass in the validator/constraint as well. I'll prob just leave it as a macro for now 🤷 |
When you're already hesitant to add something to your project because it's a separate shard, I'm even more so about such a huge change in stdlib JUST for that. =) |
|
Oh I'm definitely not saying we should refactor the whole stdlib to use a new Spec implementation. Just pointing out something that could be useful to give insight into future improvements/changes. I'd for sure rather keep things simple as my current workaround is pretty decent for what its doing. If anything, some built in feature around something like data providers could be handy. Granted I haven't put much thought into if that would even be possible or how it would work in the current implementation. |
So is a new style for the stdlib spec. There's a huge value in having one standard way to do things, even if you don't like the standard personally. |
|
FWIW I threw https://play.crystal-lang.org/#/r/9i05 together as a proof of concept I had. IDK if we'd want something like that in the stdlib, but it was fairly easy to expand the current I'd be happy to improve the implementation/feature set if this is something we'd want in the stdlib. Otherwise, I might just make a shard with it. EDIT: It has been released as https://github.com/athena-framework/spec. |
After the recent refactor of spec in #8125 we lost of a bit of functionality, which I now kind of regret.
In the past spec was simple: everything ran from top to bottom as soon as you wrote things. For example:
In the old spec we could create an object, share it in a couple of specs, then clean up. In the new spec this isn't possible because the code surrounding
itwill run before the contents ofit.Now, RSpec works this way. But in RSpec you have before/after hooks, you have
letand you have instance variables you can share in your specs.We have two options:
letto be able to share variables between specs, etc.I think I prefer the first option, only because it's simpler. Maybe we don't know focus, randomized specs and so on. The second option brings a lot more complexity, which RSpec is known for (and I don't like it).
Thoughts?
The text was updated successfully, but these errors were encountered: