celluloid / celluloid-io

UNMAINTAINED: See celluloid/celluloid#779 - Evented sockets for Celluloid actors
https://celluloid.io
MIT License
879 stars 93 forks source link

Update to RSpec 3.2 + update to use celluloid/#537 #134

Closed e2 closed 9 years ago

e2 commented 9 years ago

Last commit requires the following PR to be merged in Celluloid first: https://github.com/celluloid/celluloid/pull/596

niamster commented 9 years ago

@e2 please check c636e016382

niamster commented 9 years ago

@e2 please check my changes a186073, all tests are passed

e2 commented 9 years ago

@niamster - thanks for the 2 commits! (cherry-picked and added). I'm glad I managed to meanwhile refactor the Celluloid spec_helper into smaller files. I hope the logger stubbing wasn't too much trouble (there are extra checks in Celluloid, but they aren't necessary here) - overall stubbing the logger is tricky, and the env variables and env config weren't meant to be reused in other projects.

Sharing specs like this isn't good for maintenance - but I'm glad getting it work didn't require too much copy&paste.

@digitalextremist - I rebased this against 0.17.0-dependent. Let me know if that's ok (seemed to have a few more commits than master). This can be merged if tests pass (mostly just RSpec upgrade).

niamster commented 9 years ago

@e2 you are always welcome. Maybe it's worth to just add some generic helper to include all necessary helpers for actor spec?

e2 commented 9 years ago

@niamster - I thought about this.

Almost all Celluloid tests are "integration specs" - and almost every one of them is "full-stack", while there is no separation between "areas". Separating them may hurt Celulloid performance, which is another problem.

Otherwise you'd have to almost write a "special" version of Celluloid to be the test helper.

It's best to find a troublesome "use case" and try dealing with it.

Celluloid::IO is another story though, because it's an "extension", not a "usage". I don't know if DCell is an "extension" or a "usage" though (seems like both, really).

As for avoiding duplication in spec_helper - I don't think that's worth time pursuing, since sharing specs means sharing a responsibility (which is a bigger problem). And since the projects are in separate gems, breakage is going to happen in unexpected areas anyway (Travis is the best way to track these).

digitalextremist commented 9 years ago

@e2 I haven't been able to read your emails yet, but I did merge celluloid/celluloid#596 just now.

digitalextremist commented 9 years ago

@e2 looks like this needs to be rebased against 0.17.0-dependent and have the PR reference that branch.

digitalextremist commented 9 years ago

@e2, @niamster -- I've also been thinking about this, and I wonder if we ought to consolidate the test tools into celluloid-testing and place those in celluloid/lib/testing/* ( in the gem ) with celluloid/test.rb requiring in the suite of tools, and the shared examples. This way we can stop duplication. This seems like a good idea because more and more of those have behaves as segments which work across gems, such as celluloid-task-pooledfiber behaving like a Task.

e2 commented 9 years ago

@digitalextremist - rebased in #141

e2 commented 9 years ago

@digitalextremist - don't do that. There's no need to place those tools in celluloid/lib/testing, since they are only required by tests of other Celluloid projects. Users of Celluloid need something different. The files should be/stay in spec/support - and this patch shows how to include them in other Celluloid org projects.

e2 commented 9 years ago

@digitalextremist - avoiding duplication is hiding a bigger problem.

niamster commented 9 years ago

@e2 there's also celluloid-zmq that uses celluloid spec

e2 commented 9 years ago

@e2 there's also celluloid-zmq that uses celluloid spec

@niamster - it's probably better to sit down and invest in isolated unit tests instead, but I probably won't get to that too quickly. celluloid-zmq shouldn't be testing celluloid - it should be just testing what it adds/changes.

Integration/acceptance testing is an entirely different subject.