Fixes #24886 - fixed audit in seeds test #6058

lzap · 2018-09-11T09:38:34Z

For some reason, audit was disabled randomly for all model classes. This
has to do with how Audited gem is loaded by bundler perhaps, not sure.
This patch enables it explicitly.

It also makes seeds test faster by only seeding minimum files required
for individual tests.

theforeman-bot · 2018-09-11T09:38:44Z

Issues: #24886

lzap · 2018-09-11T09:40:53Z

db/seeds.rb

@@ -13,7 +13,7 @@ def format_errors(model = nil)
 end

 # now we load all seed files
-foreman_seeds = Dir.glob(Rails.root + 'db/seeds.d/*.rb')
+foreman_seeds = Dir.glob(Rails.root + 'db/seeds.d/*.rb').sort


Oh this was left over. Removing.

lzap · 2018-09-11T09:41:15Z

test/unit/tasks/seeds_test.rb

+      tmpl = ProvisioningTemplate.unscoped.find_by_name('Kickstart default')
+      tmpl.name = 'test'
+      tmpl.save!
+    end


This is the key thing that fixes random failures - ensures that auditing is enabled during this test.

lzap · 2018-09-11T09:42:48Z

test/test_helper.rb

+  yield
+ensure
+  klass.enable_auditing if auditing_was_enabled
+end


I don't think this is useful outside of testing context, so not putting this into model classes.

bastilian · 2018-09-11T09:52:29Z

test/unit/tasks/seeds_test.rb

+        seed_files.each do |file|
+          load File.expand_path("../../../db/seeds.d/#{file}", __dir__)
+        end
+      end


This could also be written like this, saving a few lines and DRY it up.

seed_files = ['seeds.rb'] if seed_files.empty? seed_files.each do |file| load File.expand_path("../../../db/seeds.d/#{file}", __dir__) end

mmoll · 2018-09-11T18:23:52Z

tests fail reliably now :)

tbrisker · 2018-09-12T11:42:14Z

test/test_helper.rb

+  klass.enable_auditing
+  yield
+ensure
+  klass.enable_auditing if auditing_was_enabled


shouldn't this be klass.auditing_enabled = auditing_was_enabled to ensure it is reverted to the same state as before?

This was a copy paste from audited gem, sure I can do that. :-)

Oh the method is enable_auditing and not enable_auditing= actually.

def without_auditing auditing_was_enabled = auditing_enabled disable_auditing yield ensure enable_auditing if auditing_was_enabled end def disable_auditing self.auditing_enabled = false end def enable_auditing self.auditing_enabled = true end

I think @tbrisker has a valid point here, do we want to revert this to the original state?

Again, this method was taken from audited gem, there is no enable_auditing= method available. Please elaborate what's wrong.

I am wondering if klass.disable_auditing unless auditing_was_enabled is missing.

I think this is still not resolved, original value is not being restored in ensure, I think this would be necessary "auditing_was_enabled ? klass.enable_auditing : klass.disable_auditing" in ensure block

perhaps link audited code you copied here?

NOW I get it! This code was from audited gem, but they had the opposite with_auditing". Looks like I only need this then: klass.disable_auditing unless auditing_was_enabled`

tbrisker · 2018-09-12T12:03:23Z

test/unit/tasks/seeds_test.rb

@@ -35,11 +39,11 @@ def seed
    seed

    tables.each do |model|
-      refute model.unscoped.count.zero?
+      assert_not model.unscoped.count.zero?


iiuc, the difference between refute and assert_not is that refute expects false while assert_not also accepts nil. Is there a condition in which this and other assertions you changed return nil instead of false?

Good point, I haven't found any.

tbrisker · 2018-09-12T12:03:35Z

test/unit/tasks/seeds_test.rb

@@ -14,10 +15,13 @@ class SeedsTest < ActiveSupport::TestCase
    Foreman.stubs(:in_rake?).returns(true)
  end

-  def seed
+  def seed(*seed_files)


lzap · 2018-09-18T13:35:17Z

I believe no changes are required @tbrisker however tests were failing because my previous change, fixed that.

lzap · 2018-09-25T07:39:27Z

Ping anyone can do quick merge here? I believe this is RTM. @ares or @timogoebel

lzap · 2018-10-02T07:24:34Z

Removed assert_not change and .gitignore change.

lzap · 2018-10-02T08:58:10Z

How about this?

comments were addressed

ares

Tests failres are unrelated, all comments were IMHO addressed, looks good to me too

ares · 2018-10-02T10:17:27Z

Thanks @lzap, merging!

timogoebel · 2018-10-02T11:51:41Z

... what about @tbrisker and my comment about resetting the audit state?
I believe this was still pending here.

ares · 2018-10-02T15:34:35Z

@timogoebel I believe it has been resolved, it was changed to disable unless was_enabled logic, which is I believe correct. If we enable it, we ensure it's disabled back unless it wasn't enabled alreasy

timogoebel · 2018-10-02T15:40:23Z

@ares: Oh, I see it now. Sorry for the confusion. All good. Would have merged it earlier, but needed this explanation. 👼

ares · 2018-10-02T17:18:20Z

No worries, we talked with lzap this morning on IRV and the final version was pushed just few hours ago :-) thanks for keeping eyes on what goes in!

lzap · 2018-10-03T15:40:37Z

Yeah this kind of setter/getter is confusing. But it works.

theforeman-bot added Needs testing Not yet reviewed labels Sep 11, 2018

lzap commented Sep 11, 2018

View reviewed changes

lzap force-pushed the seed-audit-faster-24886 branch from 7564084 to d641d40 Compare September 11, 2018 09:41

lzap commented Sep 11, 2018

View reviewed changes

lzap requested review from ares, tbrisker and mmoll September 11, 2018 09:43

lzap mentioned this pull request Sep 11, 2018

fixes #1459 - ipxe provisioning support #5988

Merged

bastilian reviewed Sep 11, 2018

View reviewed changes

lzap force-pushed the seed-audit-faster-24886 branch from d641d40 to 8a9ddcb Compare September 11, 2018 11:14

lzap mentioned this pull request Sep 11, 2018

Fixes #24759 - Fix failing API FactValueController tests #6013

Merged

tbrisker previously requested changes Sep 12, 2018

View reviewed changes

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels Sep 12, 2018

lzap force-pushed the seed-audit-faster-24886 branch from 8a9ddcb to e5d45b7 Compare September 18, 2018 13:34

theforeman-bot added Needs re-review and removed Waiting on contributor labels Sep 18, 2018

lzap force-pushed the seed-audit-faster-24886 branch from e5d45b7 to 1ba2c9b Compare October 2, 2018 07:24

Fixes #24886 - fixed audit in seeds test

ec9b110

lzap force-pushed the seed-audit-faster-24886 branch from 1ba2c9b to ec9b110 Compare October 2, 2018 08:58

ares approved these changes Oct 2, 2018

View reviewed changes

theforeman-bot removed the Needs re-review label Oct 2, 2018

ares merged commit bb93b26 into theforeman:develop Oct 2, 2018

lzap deleted the seed-audit-faster-24886 branch October 3, 2018 15:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #24886 - fixed audit in seeds test #6058

Fixes #24886 - fixed audit in seeds test #6058

lzap commented Sep 11, 2018

theforeman-bot commented Sep 11, 2018

lzap Sep 11, 2018

lzap Sep 11, 2018

lzap Sep 11, 2018

bastilian Sep 11, 2018

mmoll commented Sep 11, 2018

tbrisker Sep 12, 2018

lzap Sep 18, 2018

lzap Sep 18, 2018 •

edited

Loading

timogoebel Sep 25, 2018

lzap Sep 25, 2018

timogoebel Sep 25, 2018

ares Oct 2, 2018

lzap Oct 2, 2018

tbrisker Sep 12, 2018

lzap Sep 18, 2018

tbrisker Sep 12, 2018

lzap commented Sep 18, 2018

lzap commented Sep 25, 2018

lzap commented Oct 2, 2018

lzap commented Oct 2, 2018

ares left a comment

ares commented Oct 2, 2018

timogoebel commented Oct 2, 2018

ares commented Oct 2, 2018

timogoebel commented Oct 2, 2018

ares commented Oct 2, 2018

lzap commented Oct 3, 2018

Fixes #24886 - fixed audit in seeds test #6058

Fixes #24886 - fixed audit in seeds test #6058

Conversation

lzap commented Sep 11, 2018

theforeman-bot commented Sep 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmoll commented Sep 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lzap Sep 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lzap commented Sep 18, 2018

lzap commented Sep 25, 2018

lzap commented Oct 2, 2018

lzap commented Oct 2, 2018

ares left a comment

Choose a reason for hiding this comment

ares commented Oct 2, 2018

timogoebel commented Oct 2, 2018

ares commented Oct 2, 2018

timogoebel commented Oct 2, 2018

ares commented Oct 2, 2018

lzap commented Oct 3, 2018

lzap Sep 18, 2018 •

edited

Loading