Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix automate specs after MulticastLogger changes #17001

Conversation

NickLaMuro
Copy link
Member

Specs that used stub_automate_workspace would fail no because, previously, the calls to resolve_automation_object:

def self.resolve_automation_object(uri, user_obj, attr = nil, options = {}, readonly = false)
  raise "User object not passed in" unless user_obj.kind_of?(User)
  uri = create_automation_object(uri, attr, options) if attr
  options[:uri] = uri
  MiqAeWorkspaceRuntime.instantiate(uri, user_obj, :readonly => readonly).tap do |ws|
    $miq_ae_logger.debug { ws.to_expanded_xml }
  end
end

Would trigger the $miq_ae_logger.debug line and just no-op, because the underlying logger would be set to info.

This change allows the double that is instantiated in stub_automate_workspace to also receive #to_expanded_xml optionally, so that line won't cause failures.

Links

Steps for Testing/QA

This probably needs to be merged into master to allow ManageIQ/manageiq-providers-amazon#410 to pass and be a proper QA, but you can check out that branches' changes by doing the following:

$ cd /path/to/manageiq-providers-amazon
$ git apply <(curl -L https://github.com/ManageIQ/manageiq-providers-amazon/pull/410.patch)

And then update the manageiq spec helper by doing the following

$ cd spec/manageiq
$ git apply <(curl -L https://github.com/ManageIQ/manageiq/pull/17001.patch)

Specs that used `stub_automate_workspace` would fail no because,
previously, the calls to `resolve_automation_object`:

    def self.resolve_automation_object(uri, user_obj, attr = nil, options = {}, readonly = false)
      raise "User object not passed in" unless user_obj.kind_of?(User)
      uri = create_automation_object(uri, attr, options) if attr
      options[:uri] = uri
      MiqAeWorkspaceRuntime.instantiate(uri, user_obj, :readonly => readonly).tap do |ws|
        $miq_ae_logger.debug { ws.to_expanded_xml }
      end
    end

Would trigger the `$miq_ae_logger.debug` line and just no-op, because
the underlying logger would be set to info.

This change allows the `double` that is instantiated in
`stub_automate_workspace` to also receive `#to_expanded_xml` optionally,
so that line won't cause failures.
@gmcculloug
Copy link
Member

@simaishi I added the same gaprindashvili/yes and blocker labels as the PR that exposed this test failure.

@miq-bot
Copy link
Member

miq-bot commented Feb 14, 2018

Checked commit NickLaMuro@671a346 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🏆

@gmcculloug gmcculloug merged commit 0f50b52 into ManageIQ:master Feb 14, 2018
@gmcculloug gmcculloug added this to the Sprint 80 Ending Feb 26, 2018 milestone Feb 14, 2018
simaishi pushed a commit that referenced this pull request Feb 14, 2018
…lticast-logger-fallout

Fix automate specs after MulticastLogger changes
(cherry picked from commit 0f50b52)
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit 04fa2a388dd0c3508e7abfcd1a8cece9ac70e1aa
Author: Greg McCullough <gmccullo@redhat.com>
Date:   Wed Feb 14 15:28:24 2018 -0500

    Merge pull request #17001 from NickLaMuro/fix-automate-specs-after-multicast-logger-fallout
    
    Fix automate specs after MulticastLogger changes
    (cherry picked from commit 0f50b52c8c6921290a2ed6482033bc5e3fe52793)

@NickLaMuro
Copy link
Member Author

So, not that anyone is going to care, but wanted to give an explanation as to why this just showed up when it did. It was indeed after #16990 was merged, but that only exposed it. Prior to even #15392 , where the MulticastLogger was introduced, this would have been an issue IF we had our logs in DEBUG mode in our RAILS_ENV=test (apparently we don't).

For those following along at home, but need a quick refresher, the issue was with this line of code:

$miq_ae_logger.debug { ws.to_expanded_xml }

[source]

Specifically, ws in this case, was a rspec double that didn't have #to_expanded_xml stubbed. The $miq_ae_logger was then implemented differently based on changes in previous PRs:

  • Prior Logging to STDOUT in JSON format for containers #15392 (so what I only can assume is the entire existance of these stubbing mechanisms), $miq_ae_logger was just a VMDBLogger, and the level for it's instances defaults to INFO. To keep this short I only can assume that we never updated the @level of the logger in the test environment via lib/vmdb/loggers.rb, and so the line above always skipped calling the block.
  • When Logging to STDOUT in JSON format for containers #15392 was introduced, it wrapped VMDBLogger with the MulticastLogger to allow broadcasting to multiple log devices. Unfortunately, as was described in Fix MulticastLogger DEBUG mode #16990, while DEBUG was now the default log level for the MulticastLogger instance itself, it didn't replicate that level on initialization, and so the lower level VMDBLogger was still set at info. This, again, basically no-oped when hitting the code above.
  • When Fix MulticastLogger DEBUG mode #16990 was introduced, this is when the issue was exposed because there was now parity in the defaults for the log levels of $miq_ae_logger between the MulticastLogger and VMDBLogger.

So basically what I am saying is this was always busted like this, we just never had a chance where it would bumble up because our tests were never logging in debug mode until now. ¯\_(ツ)_/¯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants