Refactor our specs big time

[troessner]

Completely unrelated to @waldyr 's PR but honest to god, our spec set up is a huge pain in the ass sometimes. When one isolated change like a new smell detector causes a huge diff already then this means that we have a lot, a lot of "know it all, test it all" specs.

I believe we should start by making our classical "smelly.rb" example that leaks throughout our spec base wayyy smaller so that it actually only has one single offense.

Then we should remove our shared examples or clean them up significantly. Then we should remove all kind of duplicated specs.

Let's work on that in the future. Our spec set up is the thorn in our paw.

[mvz]

Yes. The best strategy here is probably to avoid adding complexity to the specs. So any PR should either leave the overall spec structure exactly as is, or it should remove complexity.

One think we cannot avoid is new detectors detecting new smells in our own code base. This does however provide a first test of validity for those new detectors: Do they work in practice on a relatively neat code base?

:+1: on keeping the clean and smelly example code as simple as possible.

Let's keep this issue open and we can chip away at it a bit at a time.

[waldyr]

I believe we should start by making our classical "smelly.rb" example that leaks throughout our spec base wayyy smaller so that it actually only has one single offense.

An empty class reeks of Irresponsible Module.

Just enumerating things to do for this issue so I can tackle them later:

• [ ] Make smelly.rb only reeks of Irresponsible Module.
• [ ] Remove Shared Examples.
• [ ] Seek and destroy all duplicated specs.

Am I missing something?

[mvz]

Make smelly.rb only reeks of Irresponsible Module.

I like this one.

[troessner]

Make smelly.rb only reeks of Irresponsible Module. I like this one.

Not a big fan. I don't think this example is very accessible for newbies because in real life you almost never have empty classes.

How about something that is fully functional and still kind of obviously smelly like this one:

# Dummy class
class Dummy
  # This will reek of UncommunicativeMethodName
  def x
  end
end

WDYT?

[mvz]

WDYT?

Yes, that one is better.

[waldyr]

WDYT?

I agree.

Going to work on it later.

[waldyr]

Are we going to drop all samples files except smelly.rb?

[troessner]

Are we going to drop all samples files except smelly.rb?

I believe we will keep quite a few of them. E.g. everything we use in our samples feature and also of course all the detector specific samples and so on. But we probably can get rid of 50% of all the others. Unfortunately there is no generic rule we can apply here, we need to go through every sample file, check out where it is used, decide if we can streamline / remove it and then implement the change.

[mvz]

Yes, this is going to be a long, slow process :frowning:.

[troessner]

Yes, this is going to be a long, slow process 😦.

But afterwards everything is nice and shiny! We're actually paying dearly now for years of spec mismanagement. I mean, literally every time I worked on our specs I thought "ok, that sucks, we should fix it" :)

[troessner]

Closing this one with #992 merged!