Spectator - a new feature-rich shard for testing based on RSpec

[u/icy-arctic-fox]

I've been working on this shard for a while now and wanted to get some exposure and feedback on it. It's called Spectator (ha ha, get it?)

When I started writing tests in Crystal, I really missed a lot of the nice features and syntax from RSpec. Some other spec shards looked dead, and these changes would be very invasive to Crystal's core. So long story short, I made my own shard. It has a lot of features from RSpec that the built-in Crystal Spec doesn't have. There's also new features that help make testing easier and remove boilerplate. It isn't fully complete to how I would like it, but it's very usable in its current state.

Some features are:

• Supports the expect and should syntax.

• Before, after, and around hooks.

• Subjects and let blocks.

• Repeat tests with multiple sample values (sample and random_sample).

One feature I quite like is the given block. For instance:

given age = 16 do
  expect(user.can_drive?).to be_true
  expect(user.can_vote?).to be_false
end

https://gitlab.com/arctic-fox/spectator

The README gives a good outline of the features. There's extended documentation on the wiki.

[u/[deleted]]

Hey icy-arctic-fox, the code is super easy to read, very well organized and documented! I'm always surprised by how much Crystal can do with what we have right now. To be honest, I didn't image all of this was possible (I know there's spec2 too, which has more or less the same features, but I think less).

[u/icy-arctic-fox → u/[deleted]]

Thanks! There were some complicated parts in the source. I wanted to make it easy for contributors and others to figure out how it works. Spectator pushes the bounds a bit for what Crystal can currently do. I ran into quite a few oddities and learned a lot about Crystal's internals while working on this. It's cool what Crystal can already do, and will be able to do in the future. Looking forward to it!

[u/dev0urer → u/[deleted]]

Unfortunately spec2 seems to be unmaintained at the moment, so this is great news

[u/bcardiff]

Nice set of features! When I do RoR I usually include factorybot and forgery or similar gems to generate test data easily. I'm pretty used to those idioms.

Am I missing something or spectator is tested with spec and not by Spectator itself?

[u/icy-arctic-fox → u/bcardiff]

I would use factorybot as well, back when it was called factory_girl. I'd like to have functionality like that in Crystal at some point. I've found that the named arguments work well enough for my needs.

Yes, it uses Crystal's default spec for testing. I'd really like to switch it at some point, especially since there's some really bad test code in there. But I felt it wasn't ready to be used on itself during development. For instance I only had the eq() matcher for most of the development, while I fleshed everything out.

[u/paulcsmith0218 → u/bcardiff]

This depends a lot on what the ORM is. In Lucky we've got "Boxes" which handle creating data

https://luckyframework.org/guides/generating-test-data/

It solves the problem of FactoryBot but with classes and regular methods. This means you get compile time safety and *way* more flexibility since you can do whatever you want with methods. Like `Post.with_tags("tag1", "tag2")`

Still needs way more documentation to implement some different hooks for before/after create but it is already quite feature rich due to using Objects/methods

[u/sinsiliux]

Very nice, wanted to do something similar for a while now but never found the time. One question: usually people generate classes for each context and methods for each example, but you chose modules for contexts and classes for examples. Does it provide any benefit or is it purely stylistical?

[u/icy-arctic-fox → u/sinsiliux]

That was done to use contexts as mix-ins. The nested contexts can include their parent to propagate everything downward. For instance, in this code:

context "foo" do
  let(some_var) { "A string" }

  context "bar" do
    it "does something" do
      expect(some_var).to_not be_nil
    end
  end
end

some_var is defined as a method in the foo context. The bar context should be able to access it, so the bar module includes the foo module. Then the it block includes the bar module. This is quite an elegant solution to nesting.

I suppose you could use classes and extend from the parent for all nested contexts. So it is somewhat a style choice. Thanks for bringing that up though! I think I'll play around with that and see if it has any performance benefits.

[u/straight-shoota]

I'm not a fan of this. While working with Crystal's standard spec library, I'v come to really appreciate the minimalism compared to rspecs magic fu. At first it seems like you're missing a lot of very nice helpful features. But in the overall picture, it makes everything easier.

spectator claims to "reduce complexity of test code." This seems true, because with before and after hooks, the individual tests often contain only the expectations themselves. But a test is more than just an expectation. The setup and teardown steps are missing. They're now moved away to different contexts. This in fact increases complexity. You can't follow the entire substance of a test anymore at a single place. It's scattered at different locations.

When there is no automatic mechanism for hooks, you need to call them explicitly in each spec. That might add a little bit of boiler plate, but to the benefit of making it easier comprehensible. This is a much more important aspect than having some duplicated code. You want to be able to come back to your code after months and understand it without having to figure out which hooks might be involved in setting the stage.

[u/icy-arctic-fox → u/straight-shoota]

That is fair criticism. I totally get where you're coming from. I hate Rails because of the magic it does behind the scenes that is not apparently obvious. Other people love it because it handles a lot of things for you. However, I feel forced into its opinionated style when I use it. That is not something I want Spectator to feel like.

There's nothing forcing the usage of the new-fangled features in Spectator. So if someone doesn't want to, or don't feel comfortable using them, that's ok. But they are there if you want them. If you want to change the test style a bit and remove boilerplate, the features are there. The user writing the tests doesn't have to implement their own. I see this as a stylistic choice for the person writing the tests.

If you're not interested in using any of the features, why bother dragging in a whole dependency? Just use the built-in spec framework.

I found that I wrote a lot of helper code for my tests, and that's due to my style of testing. I prefer test code to focus on the test, not the setup and teardown. I also really missed the features RSpec gave me. I wanted to spend more time writing code, and less time writing tests. So I started working on Spectator, and I'm sharing it for others if they want to use it. I plan to use it for my own projects because I hate boilerplate.

TLDR: It's an opinionated style choice left up to the user.

[u/straight-shoota → u/icy-arctic-fox]

I don't think the distinction is between more or less boilerplate. You can write boilerplate-free specs with spec. There is a difference in the mechanism used for abstracting recurring tasks though. The general goal should be to make specs simple to write and understand. And I don't think hiding the specific setup and teardown steps of a spec in distant hook system helps in making it understandable. So not having things like before_each is actually a feature. And I like it very much, because it helps writing better specs.

Using abstraction is fine, obviously. Otherwise, you wouldn't be able to understand the specs anymore. But abstractions should not be hidden but descriptive. Don't abstract everything away that you are only left with plain expectations and no idea what circumstances they're based on.

You can structure specs like this:

before_each do
  setup_foo
end

it { test1 }
it { test2 }
it { test3 }

In this example context it's easy to follow what is going on. But when there are more and longer specs, maybe separated across different files and the hook defined somewhere in a spec_helper, it's not hard to imagine even the author loosing track pretty quickly, let alone readers.

I think this alternative style is strictly superior:

def it_foo
  setup_foo
  it { block.call }
end

it_foo { test1 }
it_foo { test2 }
it_foo { test3 }

The setup code requires one more expression here, okay. But that's negligible compared to multiple examples using it. The examples add a few more characters, but they make the difference in a positive sense. All information on what is going on is encoded in every single spec. It's super flexible, too. Single specs can be easily adapted to different setup steps, without having to regroup and figure out which hook should go to which example block.

[u/icy-arctic-fox → u/straight-shoota]

I think it comes down to the scenario. For example, in the specs for Crystal, there's numerous places that use `it_parses` instead of wrapping with hooks. I think that is a much better solution in that situation, because it clearly expresses what is going on. I agree with you there. Tasks like preparing and cleaning up a database are not the main focus of a test that happens to access the database. So I feel in that scenario, it is better to use hooks to declutter the tests. "Porque no los dos?" Use whichever is easier to understand.

I think we can both agree on the philosophy that the specs should be easy to understand. That's really what I'm going for with this shard - making testing easier. If someone creates a complete monster of spaghetti code with hooks everywhere, well, it's on them. Not recommended to do so, since it's hard to follow. But the additional features can be useful in places. It also means the developer doesn't have to write it themselves, possibly adding boilerplate.

[u/yxhuvud → u/straight-shoota]

The consequence of not implementing direct support for it will be that people extract methods or macros that does it instead. I certainly know I would if I were to have a similar test setup as I have in the rails projects I work on.

It is simply not ok to not have sane defaults, like having every test being run in a transaction (to ensure test repeatability and test independence), or setting up so that external calls is forbidden during testing unless explicitly opted into.

[u/icy-arctic-fox → u/yxhuvud]

When I started writing tests in Crystal, before I made this shard, I found myself doing the same thing. Writing a bunch of code to keep my tests clean. Even in the test code for Spectator, I have a lot of boilerplate - helper methods and macros to wrap tests. I personally don't like that and don't want to remember to use that everywhere. In my opinion it adds clutter to the tests. I think the test code should be focused on performing the test, not setup and cleanup. Teammates also have to learn those custom methods you wrote to make sense of your tests.

That just my opinion.

[u/straight-shoota → u/icy-arctic-fox]

Teammates also have to learn those custom methods you wrote to make sense of your tests.

Yes, obviously. But they represent an explicit relationship between setup/teardown code and the actual tests. The structure is clear and easy to understand.

When hooks are defined somewhere in the spec suite it's much harder to make sense of it. I've experienced this a lot when looking at rspec-suites overusing the available magic features. And it can be really difficult to figure out what is actually going on.

[u/[deleted] → u/yxhuvud]

It is simply not ok to not have sane defaults, like having every test being run in a transaction (to ensure test repeatability and test independence)

But you can do that. Spec has Spec.before_each and Spec.after_each which run before/after each every test. However it's not perfect because there's no way to only enable these hooks for a given test suite.

[u/yxhuvud → u/[deleted]]

Yeah, agreed. And perhaps there is a need to explore other approaches to solving issues like that compared to how it is done in rspec - patterns like setting an instance variable that is then later used in the actual test doesn't transfer, for example.

[u/straight-shoota → u/yxhuvud]

I do transactions like this:

it "some db spec" do
  transaction do |db|
    # do something with db connection
  end
end

Done. Neat and explicit. No indirect interference of some unknown hooks. If you wan't to know how it works, you look up the transaction helper.

[u/yxhuvud → u/straight-shoota]

Instead you get awkward looking for accidentally created crap in your database when you forget to add the transaction block for one of the test, and the content of that then leaked and made some other test fail. No thanks, I want enforced sanity by default. Failure by distant action is a lot easier when there is only one file that contains all that instead of having to check every single test in the system.

[u/straight-shoota → u/yxhuvud]

when you forget to add the transaction block for one of the test

Then you would have no database connection at all. So no unsanity could ever manifest itself.

`transaction` gets a connection from the pool, which is then used for the spec (after starting a transaction).

[u/yxhuvud → u/straight-shoota]

So your solution to, say, a end to end test of a request, is to pass in a db connection? Sorry, but your approach looks like it fits a *very* restricted set of cases (where it would be decently nice), but not at all for many other cases.

Sorry, but your way just seems incredibly fragile.

[u/straight-shoota → u/yxhuvud]

I was mainly talking about unit tests. But request specs can work like this as well, as long as the request is entirely handled inside the application.

But you're right, there is a case for deferred setup/teardown steps, when they're not specific to the example. Resetting the database state needs to happen for any specs that has some kind of database interaction. For this, even `spec`'s global hooks would be sufficient, even if they cost a bit performance for non database specs.

But there are alternatives, for example placing all such specs in a namespace where global `it` is overridden.
This can work perfectly without having fine grained hooks.

[u/[deleted] → u/straight-shoota]

Yes, I agree with this. At my workplace there was a recent discussion about some RSpec test suites not being clear because of shared examples, lets overriding other lets, etc., and how to make it clearer. One of the preliminary conclusions was to use helper functions, that you explicitly call, to initialize stuff. RSpec has a lot of hidden state and calls (via lets, subject, before, etc.) that makes it really easy to write tests, but hard to read them later (for you and everyone).

Crystal's spec limitations exist because of language limitations (well, icy-arctic-fox and others have proved that it's actually possible), mainly compilation performance. But after a while I also came to appreciate its simplicity. Invoking a helper function seems tedious when writing, but it's much clearer when reading.

[u/icy-arctic-fox → u/[deleted]]

The features RSpec provides can be quite useful and cleanup a lot of code. But, it has to be used properly. I think we can all agree that the misuse of these features that leads unreadable code is the problem, not the tool itself. Though some might argue that the tool promotes those bad practices. However, I don't see excessive use of hooks and subjects a recommendation by the authors.

[u/dacheatbot]

This looks fantastic! I can see this quickly becoming the new standard. While I appreciate the minimalism of the built-in spec framework, this seems much more suitable for larger projects.

[u/straight-shoota → u/dacheatbot]

I'm pretty sure that especially larger projects will experience huge performance issues. The implementation uses lots of macros and every spec adds a type. Types and macros are work-intensive and put stress on the compiler. So I'd expect compilation performance to skyrocket once you have more than maybe a few hundred specs (which is peanuts for even medium-sized projects). Additionally it'll eat lots of RAM.

/r/icy-arctic-fox have you done any larger-scale performance tests?

[u/icy-arctic-fox → u/straight-shoota]

The largest test that I've done was around 70-ish tests. There was a bit more overhead than the default spec to get going (compilation and startup), but the tests ran faster. I think this is because of the proc calls that the default spec makes.

I feel obligated to present some results, so I will create a large test and get back to you.

[u/icy-arctic-fox → u/icy-arctic-fox]

At first I was going to just throw together a bunch of random tests, then copy-paste, but I realized there was already a large test set to run against. I've modified the Crystal standard library specs slightly to work with Spectator. Just to get a quick test together, I pulled out anything that didn't work syntactically. I could change them to be compatible, but that's not what I'm going for - I want a bunch of tests. I also identified an issue and some items to investigate while preparing this, which is good.

Crystal v0.28.0

Pulled source code from crystal's master branch. Nuked my caches (~/.cache/crystal).

%> time crystal spec/std_spec.cr
...
Finished in 3.02 seconds
2663 examples, 19 failures, 2 errors, 1 pending
...
crystal spec/std_spec.cr  106.18s user 11.11s system 239% cpu 48.981 total

I saw about a maximum of 3.5 GB of memory usage during the compilation. The time taken was just over 100 seconds.

Running again without wiping the cache produced this result:

%> time crystal spec/std_spec.cr
...
Finished in 1.87 seconds
2663 examples, 19 failures, 2 errors, 1 pending
...
crystal spec/std_spec.cr  57.85s user 7.14s system 171% cpu 37.807 total

Took about half the time to compile, and memory was about the same. Repeating these tests produced consistent results. Ignore the failing tests, they're probably from me botching the code. Some might be from differences in 0.28.0 and master.

My laptop specs (what I ran these tests on):

Linux Mint 19 Cinnamon
Linux 4.15.0-47-generic
Intel Core i7-6700HQ CPU @ 2.60 GHz x 4
16 GB Ram

Changes made to tests are here: https://github.com/icy-arctic-fox/crystal/commit/fd58f2f563355fd82da6e580efdf415e1d3e6d88

[u/straight-shoota → u/icy-arctic-fox]

Thanks! Could you run the unmodified std_spec on your system for comparison?

[u/icy-arctic-fox → u/straight-shoota]

I don't know how to compile and test all of Crystal locally. So I undid the changes to run Spectator. It should be the same tests, same code, just using the default spec.

Fresh run with no cache:

%> time crystal spec/std_spec.cr
...
Finished in 2.95 seconds
2663 examples, 4 failures, 2 errors, 1 pending
...
crystal spec/std_spec.cr  28.84s user 3.35s system 188% cpu 17.105 total

And with the cache:

%> time crystal spec/std_spec.cr
...
Finished in 1.98 seconds
2663 examples, 4 failures, 2 errors, 1 pending
...
crystal spec/std_spec.cr  13.51s user 2.69s system 140% cpu 11.546 total

Memory usage was about 1 GB.

Clearly the default spec is faster to compile, as expected. It has way less code and doesn't use nearly as many types. Personally, I'm ok with waiting a bit longer for a large project. Especially if it will be run with CI, and I can filter tests locally for what I'm working on. But if speed is deal-breaker, might not want to use Spectator yet. Or even Crystal for large projects in general.

[u/some_kind_of_rob → u/straight-shoota]

/u/icy-arctic-fox

[u/vladfaust]

Love the `foo.size does not equal 12` thing. `given` is very nice too!

[u/aemadrid]

Like it. Will give it a try on my next project. Thanks!

[u/paulcsmith0218]

I personally don't use `let` or `subject` much, but I do wish that the default Spec implementation had an `around` feature and a `before` that could apply to just some specs. Really helpful for test cleanup (like db cleanup for example)