0

I have a complex condition to assert in one of my methods:

Today it's: 

if deal.video_url.present? && deal.video_url_changed? 
  # do stuff
  # about 15 lines of code
end

Unfortunately story_step.st_video_url_changed? is giving me false positives and negatives in Rspec tests (see the issue also mentioned here: How can I test that no attribute on a model has been modified using rspec?)

Anyway, what I wish to do is modify this condition check line to become

"if we're on rails development or production environements, please check A(deal.video_url.present?) && B(deal.video_url_changed? ) but if Rails environement is Test, ONLY check A deal.video_url.present?

Of course I could write:

if Rails.env.development? || Rails.env.production?
  if deal.video_url.present? && deal.video_url_changed? 
    # do stuff
    # about 15 lines of code
  end   
else # test environment
  if deal.video_url.present? 
    # do stuff
    # about 15 lines of code
  end

But, I feel it's really not DRY to repeat twice the exact same 15 lines of code to be executed when the condition pass.

I could factorize by creating a new method

if Rails.env.development? || Rails.env.production?
  if deal.video_url.present? && deal.video_url_changed? 
    new_method_to_factorize
  end
else # test environment
  if deal.video_url.present? 
    new_method_to_factorize
  end
end

def new_method_to_factorize
  # do stuff
  # about 15 lines of code
end

But I wonder is it possible to write this more cleanly and without creating a new method?

I tried but failed with the code below

if deal.video_url.present? && (deal.video_url_changed? unless Rails.env.test?) 
  # do stuff
  # about 15 lines of code
end

EDIT

After many comments, I realized that i am trying to put crutches while I should deal with the very core of the bug. this bug was created because I found a hacky way to associate Deals and Steps. To solve the original issue, I created a new Stackoverflow question: Rails 4.2 / Rspec / rspec-retry - association belong/has_many failing

Mathieu
  • 4,587
  • 11
  • 57
  • 112
  • 5
    "Unfortunately story_step.st_video_url_changed? is giving me false positives and negatives in rspec tests" - maybe fix that instead of adding unnecessary crutches? – Sergio Tulentsev Jun 26 '18 at 14:41
  • hehe right, I'd love to but I am failing at solving the issue. I am totally aware I am doing what you say, it's actually very nice way to say it "unnecessary crutches" but so far no luck for me after a full day of work so kind of giving up:) I am going to post a Stackoverflow new question about it but it's really tied deeply to some test stack , hoping maybe I'll manage to solve it... – Mathieu Jun 26 '18 at 14:43
  • 1
    And I hate moving seperate how things are done in test and things done in dev/prod...i know, antipattern, right? :) – Mathieu Jun 26 '18 at 14:44
  • "antipattern, right?" - more like maintenance nightmare. But that too :) – Sergio Tulentsev Jun 26 '18 at 14:47
  • I'll try posting the real question then haha but maybe you'll see, it's pretty complex:) – Mathieu Jun 26 '18 at 14:48
  • "it's pretty complex" - try your best to distill it down to absolute minimum amount of code that still reproduces the problem. Who knows, maybe you'll find the root cause yourself while doing this. Here's some tips: [mcve]. – Sergio Tulentsev Jun 26 '18 at 14:50
  • Sure but the why is important and the why goes deep in the rabbit hole:) – Mathieu Jun 26 '18 at 14:50
  • @Matthieu I do not know the full details of the project, but usually I can resolve to use "mocks" instead of having to write a code that checks for the environment in the actual production code like what you are currently doing. (i.e. if "necessary", I can mock the `video_url` to be "changed" in my spec). If however, this is more of a configuration / environment "differences" between dev/test/prod, and that your code above is potentially gonna be relying on this, then I will usually have my own `config/some_configuration.yml` that contains different settings for test/dev/prod. – Jay-Ar Polidario Jun 26 '18 at 15:01
  • @Mathieu I've resolved *hundreds* of situations like this before; it can always be fixed with some restructuring of the code and/or redesigning the tests, rather than adding `if Rails.env.test?` into the code. Doing the latter is a **big** code small -- it means your tests are now testing the wrong thing! – Tom Lord Jun 26 '18 at 15:58
  • @TomLord and sergio-tulentsev: you're right, I know ity in my guts but my issue is very specific, that's why I'll try to post the real issue on SO in order not to just use cruches but walk really properly:) – Mathieu Jun 26 '18 at 15:59
  • I'm writing it right now, but it's quite hard to precisely describe as it's related to a cross-roads of factory girl+rspec-retry+my specific Dela modela attribute constraints – Mathieu Jun 26 '18 at 15:59
  • `rspec-retry` should almost certainly be irrelevant. (And using that library is a bit of a hack; it means you must have flaky intermittent failures in your test suite. Again, it's better to fix the root cause instead of sticking a plaster over it.) At a guess, I would say this is to do with whether or not the `deal` is a *new* record. But I'd need to see a [mcve] (preferably with a little more context around what this method is supposed to be doing) to say anything for certain. – Tom Lord Jun 26 '18 at 16:05
  • no its is very relevant as it's the reason I don't use the more "common" way of creating associuated objects using Factory Girl's transient, and instead doing it myself, leading to a failure in assocating objects...more here below – Mathieu Jun 26 '18 at 16:06
  • Here is the related/original bug : https://stackoverflow.com/questions/51047204/rails-4-2-rspec-rspec-retry-association-belong-has-many-failing – Mathieu Jun 26 '18 at 16:07

1 Answers1

0

Here is a quick and very dirty solution:

if deal.video_url.present? && !Rails.env.test? && deal.video_url_changed?
  # do stuff
end

But as mentioned above, I strongly advise that you do not do this.

Redesign the code/and or the tests. Don't munge the code to get tests passing - it means your tests are now testing the wrong thing!

Tom Lord
  • 27,404
  • 4
  • 50
  • 77
  • thanks, interesting to know but as advised, I posted the real core issue, whose hacky solution...resulted in this bug where _changed was giving false negative. See https://stackoverflow.com/questions/51047204/rails-4-2-rspec-rspec-retry-association-belong-has-many-failing – Mathieu Jun 26 '18 at 16:09
  • About this dirty way: it's not exactly what I would need anyway because I need to wlays check deal.video_url.present?, even on a test environment. So in your technique a video might be present but if we're on a test environement the content of the conditon won't be executed, while I need it to be executed. It's only the condition deal.video_url_changed? that needs to be constrained to the dev and prod environmentds. thats was my difficulty. – Mathieu Jun 26 '18 at 16:11
  • *"I need to always `check deal.video_url.present?`"* -- Yes, that's exactly what I've done above. `deal.video_url.present?` will **always be executed**. The `&&` operator "short-circuits" at the first `falsey` value, which means `deal.video_url_changed?` will never be executed in test environment, but the first query will. – Tom Lord Jun 26 '18 at 16:14
  • Damn, that negation was giving me troubles this afternoon. So, in test env, `!Rails.env.test?` will turn the whole expression to false and "stuff" won't happen. When Mathieu does need it to happen. – Sergio Tulentsev Jun 26 '18 at 16:44
  • `if deal.video_url.present? && (Rails.env.test? || deal.video_url_changed?)` - that's more like it. – Sergio Tulentsev Jun 26 '18 at 16:52
  • @SergioTulentsev exactly sth was bugging me too – Mathieu Jun 26 '18 at 17:18