Support for Rails Validations

[tjchambers]

I am trying to use this gem on a moderate sized rails application that is a conversion from a large PHP application. I am new to Rails and Ruby (one year) but have 40+ years of dev experience in dozens of languages. Did some mutation testing about 20 years ago in Fortran and want to be a major contributor here to this effort - particularly as to how it can apply to a real world Rails app.

I am having good success but falling apart in one area. My Rails app relies on detection using validations and has a few STI models which validate at a higher class tier. When I run the bottom class definition - a very simple one - I get these errors during the initial phase:

Cannot find definition of: Aoth2._commit_callbacks in /Users/tj/.rvm/gems/ruby-2.0.0-p195/gems/activesupport-4.0.0/lib/active_support/core_ext/class/attribute.rb:87
Cannot find definition of: Aoth2._save_callbacks in /Users/tj/.rvm/gems/ruby-2.0.0-p195/gems/activesupport-4.0.0/lib/active_support/core_ext/class/attribute.rb:87
Cannot find definition of: Aoth2._validate_callbacks in /Users/tj/.rvm/gems/ruby-2.0.0-p195/gems/activesupport-4.0.0/lib/active_support/core_ext/class/attribute.rb:87
Cannot find definition of: Aoth2._validators in /Users/tj/.rvm/gems/ruby-2.0.0-p195/gems/activesupport-4.0.0/lib/active_support/core_ext/class/attribute.rb:87

My inference is if the callbacks were "found" they would kill the mutations. I don't think it is proper to instrument my spec with the same validations as then it would not be DRY, and would be a fragile spec (2 places to change if I change the validation rules), nor in multiple places of code. So I hope my effort to have one set of active_support validations is proper, and if so I want to help see how they can play a role in the mutation testing like they do in real app usage.

I am a strong supporter of this gem, and while I lack confidence right now in providing patches, I am really good at finding bugs and doing testing. I want to be a major contributor here so with that I invite guidance.

[mockdeep]

I generally just ignore those messages. It's looking for method definitions within your file, and when it doesn't find them for superclass methods it just moves on and doesn't bother mutating them. I don't think mutant works on class level declarations like associations and validations, yet.

[tjchambers]

Perhaps I wasn't clear. My issue is not that I expect to have the validations mutated - quite the contrary. I only expect the source in the current source file to be mutated. However the validations are a key element of using _bevalid in the spec. And since these are not discovered, my inference, which may be inaccurate, but matches my experience, is that the validations are ignored. Hence by ignoring them, the role they play in the spec file is ignored, and therefore the spec passes, when if the validations WERE in play, then the spec would not pass and the mutation would be killed.

spec:

zyz.should be_valid 

...ends up always valid.

Since the mutation stays alive due to the validations not being in play during the spec run after the mutation, I get a false positive of a missing test, a test which I never intend to put in because that would be duplicating the validations.

Please let me know if this does not make sense.

[mockdeep]

It's a little hard to understand what you're saying. What I seem to be hearing is that your validations aren't running during mutations, otherwise they would cause the specs to fail. Have you tried editing your code to match what the mutation does and then running the spec manually? Mutant doesn't disable validations, or do anything to change the behavior of code except for the single mutation shown in the diff. If it's not behaving as expected, there might be a number of reasons:

1) Mutant is actually highlighting an inaccurate assumption about how your code works. 2) Mutant isn't running the specs you think it is. It tries to be smart in choosing which spec groups to execute so that you don't end up running your entire test suite against it. 3) Mutant and Rails are not playing nice. I've documented some of my experience in the README here: https://github.com/mockdeep/mutant-rails

[mbj]

@tjchambers First: Thx for your precise definition of your feature request!

Mutant is a tool to make sure the public interface of your objects are well specified. And specs of these public interfaces can cover all mutations in private code.

If you use a DSL that creates a public interface the calling DSL itself must be mutated. So IMHO we need a mutant extension that is able to find this AR specific DSL, mutate it (in a DSL specific way?) and make mutations in specs for that generated public interface FAIL to kill mutations.

AFAIK we need a mutant-rails that adds this behavior to mutant. I'd not ship this code in main mutant code to stay focused. Also I dont like AR/rails code base / VM initialization strategy and lots of the mindset/devstyle around rails. So I'll not infect mutant core with all those workarounds needed to support this environment. But I'll support externalized support in another gem that does this ugly stuff. (EDIT)

I'd suggest you join #mutant on freenode to discuss next steps. Also adding generic support for DSL mutations needs to mutant itself would be handy. I have lots of request for other DSLs, dm-validations, ducktrap, virtus, aequitas, ...

[tjchambers]

@mockdeep Thanks for the reply. You ARE hearing me say I do not think the validations are running during my mutations. It could be any of the options you portrayed. Since you believe they "should" be running I am going to try and simplify my case and confirm that pro or con.

@mbj I concur. I will do all I can to discuss and participate, finding this toolset valuable both to my important business project and my education re: the Ruby way. Be advised I do have a day job, but this is important to it's success or so I am discovering.

[mbj]

@tjchambers Thx. BTW I'm using mutant for some of my clients projects already with big success. I dont aim 100% mutation coverage. But on important domain objects it became mandatory for me to make 100% sure I dont miss anything obvious. Especially the classes dealing with money ;)

I'd love a good rails-environment support. We can talk into detail later. I'm currently on a vacation, but I can easily join channel at any time. But ping me early enough. [EDIT finishing that statement]

@mockdeep I noticed your mutant-rails gem, and I think it is a step in the correct direction, but AFAIK far from an v0.0.1.

[mockdeep]

@mbj Ha, well technically, it's at v0.0.2, but yeah, there's not much to it. Right now I've just been using the readme to document how I use mutant. Unfortunately, I don't have the time or knowhow to quite dig into it at the moment.

[dkubb]

@mbj would mutating a DSL be any different from mutating a method? I mean obviously there's different nodes, but conceptually it should be the same and we should be able to use most of the same mutators with class-level code.

If I have something like:

validates :name, presence: true

Then it should be mutated to:

validates :random_symbol, presence: true
validates :name, random_symbol: true
validates :name, presence: false
validates :name, presence: nil
validates :name
validates nil, presence: nil
validates nil
validates
nil
# ... and so on ...

The ones that are invalid will cause errors and the ones that are valid, but untested, will cause the specs to fail.

What other parts of mutant would need to be updated to support class level mutations? Are there some fundamental design assumptions that would make it particularly difficult?

[tjchambers]

@dkubb Those look like a very good start on mutations re: validates. I will take a shot at some of my own as well to have as complete a set of these.

@mockdeep To be clear my validations are on a superclass of the class I am mutating. So they are not fair game to be directly mutated themselves in any semblance of what @dkubb was spec'ing even if this is/was possible. They are not presented in the same source.

But for the record I do believe that the validations ARE in fact in play after further testing. For example my expectations WERE INCORRECT in how a mutated AR statement in my code would behave and therefore "thought" the validations were failing. An example would be a statement like

validates_associated :measure

which I expected would test the association existed to a valid Measure object. It was! However the code...

self.measure = Measure.universal.find_by(unit: 'pts')

was mutated to a variety of altered values which included

self.measure = Measure.find_by(nil)

which I inferred would not find anything. In fact this does find the first record and therefore finds "a" record, just not the correct one. That passes the validation. This leads me down a tough path in that in order for that mutant to be killed I need in effect to replicate the code in my spec (or at least something similar) like

xxx.measure.should == Measure.universal.find_by(unit: 'pts')

This feels brittle, as if I make the decision to change that in the code, it breaks the unit test. I again declare I am a noobie at this - both Ruby, Rails AND rspec. And my tests pass as long as I do not mutate, and the test is implicitly relying on the validates_associated to test that measure_id is assigned to something proper. But adding the mutation still passes the test.

The bottomline - which I offer to get feedback mostly - is my expectation that when using mutant I should be proving that:

1) I have complete code coverage for the testable attributes of the code itself in the spec (meaning I cannot make a non-trivial change to the code and it be missed by my spec) 2) I have properly working code to the best the spec can test it 3) If 1) and 2) are true then I should not have to alter code, or spec, to just prevent a mutation from staying alive when the code (with it's validations) or spec are sufficient. My inference is that to satisfy certain mutation tests I may be compelled to make more brittle test specs than I should.

This is not a condemnation of mutant or the practice itself. This is me declaring my understanding in an effort to be educated. I couldn't be MORE behind using this and improving this where possible.

[snusnu]

@tjchambers I'll just quickly present my take on your 3 points above

1) Indeed, using our devtools first checks if line coverage is at 100% and will only invoke mutant if that's the case

2) That's kina implied by 1)

3) Ideally yes. There are quite some cases where not adding a new spec kills the remaining mutations, but a refactoring/simplification of the original code, does.

I have not looked at your specific problem in this ticket deeply, but one thing that comes to mind, is that you could expose a dedicated method to use for your validation DSL call. Something like: Measure.with_universal_unit (I couldn't come up with a better name here, but the point would be that it is a parameterless method (which has fewer(no?) mutations)

Hope that helps ...

[dkubb]

@tjchambers you can look at the tests under https://github.com/mbj/mutant/tree/master/spec/unit/mutant/mutator (especially under node/) to get a sense of what mutations mutant already performs. Mutant isn't context sensitive, it more or less mutates specific kinds of nodes in the ast the same way. The main problem right now is that it only mutates nodes within methods, not at the class level.

RE: brittle specs. You have to decide how far to take mutation testing. There are a few cases where the only option is to specify something in a bit more detail than you otherwise would (like exception messages), but for the most part it's not too bad.

I do think some of the pain you're feeling is due to AR itself though. You can't easily decouple the domain object from the persistence engine, so you're forced to either test them together (which makes for slow tests), or stub out some of AR's internals making for brittle tests because it depends on non-public interfaces.

There's almost a whole industry of people working around the issues in AR so I won't repeat the techniques they talk about, nor could I without writing for days ;), but one possible solution is to have your tests assert the specific behaviour you care about only, namely that the current measurement uses the pts unit, eg:

subject.measure.unit.should == 'pts'

There are probably better solutions that involve larger changes to how you're using AR. These specific problems are what prompted us to work on ROM (Ruby Object Mapper), of which mutant is our most important supporting tool.

[mockdeep]

@tjchambers I actually find that a lot of times mutant causes me to want to alter my code because it shows me that I have unnecessary code or I've made inaccurate assumptions about how it works.

I wouldn't say that in this case the test is particularly brittle. IMO, a test is brittle if it breaks often when changing unrelated code. In this case, the test breaks when you change the exact code under test, which is the sort of behavior you want from a test. Mutation testing definitely encourages you to be more precise about what you're testing.

@dkubb's solution gives it a little bit more flexibility, but in both cases there is still a caveat in that find_by(nil) won't necessarily return the same record every time, so it might occasionally sneak a mutation under the radar by actually returning the correct measure. This could be resolved by stubbing out find_by (which is a horrendous idea, if you were to ask me), but I'd probably just leave it as is. If this pts measure is something I were inclined to use often, I'd probably refactor to use something like this, though it just moves the caveat into the Measure model:

class Measure < ActiveRecord::Base
  def self.pts
    universal.find_by(unit: 'pts')
  end
end

[tjchambers]

@mockdeep What you touched on is exactly the great experience I am having using this toolset. But I do have trouble writing a line of code and then a line to test that I wrote that line of code successfully. I need to get beyond that. I am finding a number of patterns of my own behavior that are highlighted by mutation testing. It may be worthy of me collecting this as a tips and techniques list for those who find themselves going down this path.

Your suggestion for refactoring is a wise one, and I am not used to having so much refactoring flexibility yet as I have been working with cruder languages than Ruby that made refactoring a chore. I in no way are inclined to stub out find_by.

[mockdeep]

@tjchambers Yeah, it feels strange at first. Especially as your code improves, it starts to feel pretty pedantic to write a spec for a one-line method. But it's worth it. Not only to confirm your assumptions now, but also to provide security against future breaking changes, as well as documenting your intentions in a self-proving fashion. The ruby ideal is actually writing the test before writing the code.

[tjchambers]

@mockdeep Almost certainly off-topic here, but what excites me about mutation testing in general is it IS a bit like predicting the future. The future of alterations to the code, and application of the code in practice, which is explored in an automated way, challenges today my ability to spec the expectations of the code. It transforms the code to a sufficiently high and non-redundant quality as to be far more secure in the knowledge it has less chance of breaking, especially breaking unknowingly.

That is a style of "testing" that reaches far and wide from the essential testing that proves the happy path works. I am continually amazed at how mutation testing proves my code and my tests incomplete in critical ways. Spoken by someone who was coding before there was 8-bit ASCII.

[tansaku]

Very interesting discussion - is there a recommended way to use mutant with rails other than using the somewhat outdated mutant-rails gem?

[mbj]

@tansaku I have to admit I was very lazy providing exhaustive documentation about using mutant with rails. Thx for your PR.

[tjchambers]

Adding that eventually my above example pushed to DSL.

scope :pts, -> { universal.find_by(unit: 'pts') }

which eliminated the class method entirely. And the associated mutations. For now.