Rspec 3 support

[mooreniemi]

The origin of this PR is that rspec-console as is doesn't support RSpec 3's new API for shared_examples. If you want, of course you can ignore this PR, and just write your own implementation of supporting both (or choosing to stop supporting RSpec 2).

But, in a naive hope that I might be able to contribute to this project, I ended up doing a bit of rearranging and housekeeping and spec writing while I was working on supporting RSpec 3's shared_examples. I've also confirmed it still works under RSpec 2.

I'm happy to make further changes to better match your vision for this project. I ask that you don't do what you did last time, however, and simply copy paste parts of my code refactored to your style. It means I don't get credit, and don't get a chance to learn from you.

Best, Alex

[nviennot]

Hi Alex :)

Quick comment: the Exceptions are not getting loaded, so when you complain about the < 2.10 rspec version, something funny will happen.

With respect to credits, in the commits in master it says "Unrecognized author. If this is you, make sure amooreniemi@zipcar.com is associated with your account. Click to add email addresses in your account settings." -- So add this email to your account! this way you'll show up in the list of contributors in https://github.com/nviennot/rspec-console/graphs/contributors I'll also add you in the gem author list and in the readme :)

I will, however, swash all your commit into one. You will be the author of the commit, but I might be the committer -- I'm not sure what's the problem with this

[mooreniemi]

@nviennot Oh shoot, you're totally right, I forgot to load the damn things. I'll make that change.

And thanks for the head's up about my email -- I am adding that now. :)

[mooreniemi]

I've fixed the exceptions issue. I'd like to finish a few more tests to bring coverage up past 90%, but what I have submitted so far can be merged if you want. Thanks again.

[mooreniemi]

Take a look at the current diff and let me know what you think. Most of the style changes you'll see were suggestions from ruby-lint or rubocop, fwiw.

[nviennot]

Cool. So I really don't respect the opinion of tools like ruby-lint or rubycop. Sometimes (often?), they are plain wrong. I might have to reformat the code to my style, just letting you know. I will however make it in a separate commit.

I glanced at your changes, and they look good :+1:

I'm quickly looking at the tests, and it seems that these are only unit tests, with no integration tests. Unit tests are not very useful in our situation, because when a new RSpec version comes out, we would have no way to tell rspec-cnsole is still working. (e.g. with a new API of shared examples). Another way to look at the problem is to look at the amount of stubbing in the tests. It's really high. Seeing the test suite going green is not really saying that rspec-console is actually working for real. What would be useful though, is to write some rspec tests, with some shared examples, that would be run with rspec-console, multiple times, and see that everything works as intended.

However, for this pass, I'm okay taking the code in, and postponing the tests for later, as it's some fair amount of work. (and what sucks is that we cannot use rspec to write the tests, because since we are fiddling with rspec, we can't use it, so we might have to use minitest or something like this).

[mooreniemi]

Yeah, def agree on high stubbage. It def signals lack of integration tests. Didn't even try modeling that yet, and rails prob deserves the same as a boundary.

I worry about your view on community tools. If they are wrong, fix the tools. But if the overhead of removing standardized styles is one you wanna take on, go on with your bad self. ;)

[nviennot]

If I had to fix everything I see was wrong on the internet, I wouldn't do much of what I care about.

Same with codeclimate, it's not useful to me. It's okay to have different points of view. I'm not going to force my views on others, and tell you that it's not useful. If you like it and if it make you feel good about yourself, it's cool! I'm okay with basic style checking, but you always have to see things in context, but stuff like "break up this method, because it's too complex" is not okay. Programs are not that good at writing programs.

I'll merge your code, thanks!

[mooreniemi]

@nviennot hehe yes, differences abound.

as for static analysis, i disagree deeply, but as you say, if i try to fix everyone i see wrong on the internet..! ;)

thanks again for the merge.

[mooreniemi]

(in other words, if my logic cant be communicated via a program, i think my logic is in the wrong, not the program!)

[nviennot]

Okay, so that's where we differ. My logic can't be communicated via a program in all cases, because often, I use my aesthetic sensitivity to gauge the quality of code. I use my experience and feelings to guide me. When given two solutions A and B, I'll be able to argue for one or the other, but I won't be able to write a generic program that always choose A or B correctly. Sometimes, you have to pick between inheritance, mixins, middlewares, your_own_thing. Sometimes you have to pick between a declarative, or an explicit API. Sometimes you have to name things properly. There are many design decision that I have to make along the way that will have deep implications down the line. The cyclomatic complexity of a method is not one of them.

[mooreniemi]

well lemme put it this way, if my logic cant be expressed programmatically, it prob isn't so good. my background is as an artist, and as one i'd say, i obey certain formal principles, engage in certain logics, to be parsed by an audience (or to interrupt their ability to parse me).

what you seem to be really referring to, imo, is that you have to acknowledge that part of the "shared state" and even many of the stored procedures of your program are in a programmer. this is where i really am just stating an opinion, but one i think i can marshal support for: you want to minimize that state as much as possible.

static analysis can help with a lot of that. an over-complicated method, for instance, is demonstrably harder for a person to load into memory. (human memory i mean!) overly generic names are easily confused by people (see domain driven design for more on that, too). long lines require additional inputs to fully parseable by a human (scrolling, etc).

that's just one direction to take it though. you could also render it irrelevant by using like, the intentional programming model, and make everyone do their own DSL loaded into an IDE that's actually tracking via AST or whatever. (see unison, too.) but right now i think that's less "do-able" in the industry, so i stick with the former.

fun to talk about, anyway!

[nviennot]

:)

So I've refactored the code, written integrations tests for 2.10.x and 3.x (more or less). It runs on https://travis-ci.org/nviennot/rspec-console

I've removed a few things that I didn't understand, so maybe stuff will start to break. It would be cool if you could test it :)

[mooreniemi]

@nviennot awesome -- and nice to see it on travis, tests are prob pretty useless without ci. are you going to actually merge this pr? (do i need to fix the conflicts?)

[nviennot]

I did: https://github.com/nviennot/rspec-console/commit/a73c7c899aa523b34190f73dd80c5865fe2a399d

But if you look at the commit history, I ended up refactoring on top of it

[mooreniemi]

ah, hmm, when i pulled upstream master and run rspec i see only 11 examples, all fail. did you remove the unit tests i did? also lol at you inlining tons back in. throws up hands in defeat especially the recording proxy, that would simplify things a lot to keep broken out. (config_cache has much better naming btw, fwiw.)

[mooreniemi]

also i didnt check yet, but did you bump the version for rubygems? could prob add a hook on travis too

[nviennot]

ah, hmm, when i pulled upstream master and run rspec i see only 11 examples, all fail.

I've added a commit https://github.com/nviennot/rspec-console/commit/eb4d3d0fe5c0c6ab6f93f277e164170980fe9aa6 so that you can only run the tests with minitest through rake. the rspec tests are actually just fixtures for the integration tests.

did you remove the unit tests i did.

Yes, I removed them in https://github.com/nviennot/rspec-console/commit/5e6d635d3a438b34c7b28129b27f3d8f7de65919 as we can't use rspec for testing (because it's being tested and instrumented). The unit tests would hinder future maintenance (they would make refactoring much harder for example), without really adding any confidence that things are actually working the way they are supposed to. It feels the unit tests were actually saying the same thing as the implementation. So if the implementation is wrong, the unit tests would be validating the wrong implementation. If that makes any sense. That's why I'm a big fan of integration tests.

also lol at you inlining tons back in. throws up hands in defeat especially the recording proxy, that would simplify things a lot to keep broken out. (config_cache has much better naming btw, fwiw.)

I get that you dislike my style, and I'm okay with that. I'm not sure we have the same criteria for what simple means.

also i didnt check yet, but did you bump the version for rubygem

I did but haven't pushed the gem yet.

could prob add a hook on travis too

Travis only works with github hooks, so that's already done if that's what you meant.

While refactoring, I've improved a few things:

1) The reporter method should not be replayed on the rspec config, as it caches the output_stream of the rspec config which defaults to $stdout. This would have been problematic if the user tries to configure output_stream with something else than $stdout.

2) the shared_example_group_registry of rspec3 was incorrectly dup: it's a class instance, and the shared examples lived in a hash stored as an instance variable. dup on the registry did not dup the internal hash (dup vs deep_dup). This was fixed by duping the actual hash of shared examples.

3) The code no longer assumes all the shared example would be in the :main registry, but handles all contexts.

4) Exceptions raised when dealing with shared examples are now printed instead of silently discarded.

5) RSpec.configuration= method was gone, which was problematic for some versions of rspec2. (the integration tests caught it).

6) I've simplify the runner. Turns out there is no need to distinguish rspec2 vs rspec3 with RSpec::Runner.run().

[mooreniemi]

Hmm, curious why you think it's unsafe to test the rspec within rspec. It causes some inconveniences, but I don't think it was problematic in any way.

5: I figured. I saw this behavior was being supported, but without any specs originally for it, I couldn't see why. 6: nice. 2: hmm, how was it working then? 1: I wondered why it defaulted that way, but glad to see you clarify the expectation.

Anyway, I enjoyed trying to contribute. Thanks again for the resource. Do let me know when you update the gem version!

[nviennot]

It's not that it's unsafe to use rspec, but since we are fiddling with its configuration, redirecting the stdout, doing a bunch of weird things to it when doing the integration tests, we can't use rspec to do the integration tests. (Don't get me wrong, I don't like minitest).

5: yeah, me neither. but the integration tests made it apparent that we needed it

2: it was working but I'm pretty sure that if the test you ran had inline shared examples defined in it (as opposed to shared examples defined from the configuration stage), you would inherit the inlined shared examples on future runs, which wouldn't necessarily be harmful, but wouldn't be intentional either.

1: the bug showed up while doing integrations tests because I'm parsing the stdout to see if rspec behaved accordingly, otherwise I wouldn't have noticed.

Thank you for the contributions, now we have a solid rspec-console.

I just pushed the gem.

[nviennot]

Oh snap. Looks like there are some issues: https://travis-ci.org/nviennot/rspec-console/jobs/64308691 :)

I'm not sure why the tests related to shared examples are passing.... :(

[mooreniemi]

Doh! Shouldn't be too hard to resolve though -- you on it or want a hand?

5: Ahhhh, yeah totally didn't test that case.

FWIW, I have not found unit tests to be a hindrance to refactoring, curious to hear how that was the case for you. I can see how like, overly granular tests could be as brittle as overly general integration tests, maybe that is what you mean?

[mooreniemi]

Sidebar: as for style, I too often find god objects and so I try to obey the law "could this object be smaller?" Or I guess that's Law of Demeter? I forget offhand. I often find this has an advantage of surfacing cases I can build generically on later. I think this is semi-related to privileging unit tests (with contract tests), which are sometimes, imo, actually disposable (many tests I do in TDD I remove as I go along). That's just my viewpoint though, no biggie.

[nviennot]

I'm on it, the bug appears to be benign, and only with 2.14.x, not 2.10.x.

So my rule of thumb is always to test the implementation through the defined API of whatever the project is trying to implement. The internals don't matter. For example, you had a test https://github.com/nviennot/rspec-console/blob/a73c7c899aa523b34190f73dd80c5865fe2a399d/spec/rspec-console/config_cache_spec.rb#L24 that says that some method should be called exactly 3 times. Why? If I refactor some code and the test fails because the method is called only twice, did I break something?

Or this one: https://github.com/nviennot/rspec-console/blob/a73c7c899aa523b34190f73dd80c5865fe2a399d/spec/rspec-console/config_cache_spec.rb#L49-L50 -- If I put the version method in a Util module, I have to fix that test. What if I change the version of rspec?

Or here: https://github.com/nviennot/rspec-console/blob/a73c7c899aa523b34190f73dd80c5865fe2a399d/spec/rspec-console/config_cache_spec.rb#L53 -- what if I want to use deep_dup or clone instead of dup. The test will fail, but I didn't break anything?

Or here: https://github.com/nviennot/rspec-console/blob/a73c7c899aa523b34190f73dd80c5865fe2a399d/spec/rspec-console/proxy_spec.rb#L10 -- I refactored the Proxy to not save {:method => method, :args => args, :block => block}, but to save [method, args, block]. I would have had to change that test, even though I'm not breaking anything.

Often, these unit tests are hard to write (much harder to write compared to the implementation), and they don't necessarily prevent bugs.

I only unit test for things that are inherently complex. For example, say I implement a sorting algorithm in some Util module to sort some array in some weird way, I'll unit test it. (Unless I can't test it through an integration test).

Further, when you want to remove dead code, it's easier to do when you don't have unit tests. If everything stays green, the code is not useful for any of the implemented features. But if you have some unit test that fails, you are not so sure if you can remove it.

Also, stubbing external API/gems is very dangerous. When you upgrade a gem, you have no clue if your stuff still works because you stubbed the gem's API which might have changed. (e.g. https://github.com/nviennot/rspec-console/blob/a73c7c899aa523b34190f73dd80c5865fe2a399d/spec/rspec-console/config_cache_spec.rb#L75-L78)

Now to address your concern with big classes, I agree with you. However, splitting a class for the sake of it is not useful if there is some tight coupling between the two split classes. When you read the code, you have to toggle back and forth between the two files to understand what the code is doing. So I tend to prefer to minimize context switching. For code that is unrelated, splitting makes sense. For example .version could be in a Util module. It has no place in the rspec-cache class. But the Proxy class, this is a private class of the RspecCache class. It's not used anywhere else, and it's fairly coupled with the rest of the code. So when I read self.config_proxy.recorded_message.each { } in the code, I can just look up and see what format to expect (is it a hash? is it an array?).

For stuff like extracting Exceptions in classes, I only do it when I expect the user to programmatically catch it, or if it's going to show up in production. Otherwise, why bother. raise VersionError is not cool, because I loose the semantics of the exception message. For example, I changed the min version to be 2.10.0 instead of 2.9.10 (because I couldn't find it on rubygems.org). If I had done the refactoring later, I might not have though about changing the exception message, which would have been erroneous.

To summarize, I much prefer related code to be together to avoid brain context switches. Splitting in classes does not change the amount of code you have to load in your brain to fully understand the code.

[mooreniemi]

Yep, the expect things would've been changed to allows in the end, which don't care about calling number (no assertion). I've found very little utility in expectation on call number, except when I'm working and want to make sure I fully understand the execution path.

And as I admitted before, stubbing is problematic. It is also not the only way to do unit tests. Just an easy first step toward actual dependency injection which is made easier by extractions. Ideally, to me a unit test takes the real input to the function, which perhaps means for me it isn't as different from an integration test? Though what you seem to be describing is the necessity of exercising the "full path" of the "real use cases", esp in regards to removing dead code. Unit tests can def give false positives there, but integration tests can give false negatives. (And I worry more about those usually.) That's why I don't see it as an either or thing, fwiw.

The thing about context switching of files I think is a little bit of a MacGuffin. It's hard to say in Sublime with origami, or in vim with, well, vim (;)) that you can't easily have a couple files open or different views to the same files. I'd say "oh this is easier all in the same place" is a way of saying "my tools don't really allow separation between views on the code and the actual file org of the code", and I don't think that's true for us mostly these days.

In this case, consider that a method could've wrapped the RecordingProxy the way ConfigCache is wrapped in RSpecState. In which case you merely need understand the return type of that method, not the internals of another class. Likewise, if for some reason you wanted to later on introduce a PartialRecording or some other option, it'd be easy to do, by conditionally selecting a constructor in the method. Obviously not a big deal here, just shooting the shit at this point as it's fun.

[nviennot]

Even with the best tool, you don't necessarily open the file. (e.g. the VersionError). Why add friction when not necessary? Why add a method when you don't need to :) Anyway.

I've pushed the new code and published the updated gem. It looks like this: https://github.com/nviennot/rspec-console/blob/master/lib/rspec-console/config_cache.rb#L71-L93