Suppress Warnings from CombinePDF in CI

[stevenjcumming]

Summary

CombinePDF logs a warning "Couldn't connect reference for..." when NULL objects have empty values. NULL objects do not necessarily indicate that there is an error hence the warning. source

In order to reduce the log output in CI the warning is suppressed. Even if there is an error, the test itself will catch this.

Tasks

• If ENV['CI'] is true, suppress CombinePDF warning.

Related issue(s)

• CombinePDF Log Messages in Tests va.gov-team#78425

Testing done

• unit testing modules/claims_api and modules/simple_forms_api`
• manual examination of combined/generated pdf

What areas of the site does it impact?

• modules/claims_api
• modules/simple_forms_api

Acceptance Criteria

• PDF is still generated and combined
• RSpec log output doesn't contain the warning if CI=true
• RSpec log output does contain the warning if CI=false or not present
• Exceptions still result in failing tests

[ericboehs]

Have we determined the root cause here?

[ericboehs]

It sounds innocuous. Should we just ignore it everywhere?

[pennja]

Hello, I'm not on y'all's team but I've also been researching the cause of these messages. This is the most pertinent conversation/documentation I've been able to find on the topic. From what they can tell, the cause is most likely different PDF authoring tools assigning NULL objects. The warnings happen in case this in unintentional.

boazsegev/combine_pdf#15

Here is the exact line that appears to be setting the warning:

https://github.com/boazsegev/combine_pdf/blob/master/lib/combine_pdf/parser.rb#L664

[stevenjcumming]

Root cause: there's NULL objects. The raw data may not contain some information such as an apartment number, so when it's transformed to "page data" there's nil values:

:"F[0].Page_1[0].Claimants_MailingAddress_ApartmentOrUnitNumber[1]"=>nil,

Also by temporarily I meant the suppression is limited to the #combine_pdf method. Poor wording on my part.

[nihil2501]

@stevenjcumming Rather than manipulate the global variable, could we instead prepend a warn method in modules/claims_api/config/initializers?

if ENV['CI'] == 'true'
  CombinePDF::PDFParser.prepend(Module.new do
    SUPPRESSIBLE_WARNING = "Couldn't connect reference for"

    def warn(*msgs, **)
      msgs.reject! { _1.start_with? SUPPRESSIBLE_WARNING }
      super
    end
  end)
end

Is this warning always suppressible no matter who uses this library, or only in the exact three places you patched?
If so, I find this warning distracting in my local test environment too. Could we also suppress it there?

Thanks for addressing this!

[stevenjcumming]

@stevenjcumming Rather than manipulate the global variable, could we instead prepend a warn method in modules/claims_api/config/initializers?

if ENV['CI'] == 'true'
  CombinePDF::PDFParser.prepend(Module.new do
    SUPPRESSIBLE_WARNING = "Couldn't connect reference for"

    def warn(*msgs, **)
      msgs.reject! { _1.start_with? SUPPRESSIBLE_WARNING }
      super
    end
  end)
end

Is this warning always suppressible no matter who uses this library, or only in the exact three places you patched? If so, I find this warning distracting in my local test environment too. Could we also suppress it there?

Thanks for addressing this!

@nihil2501 I thought it might be better to keep this more visible since it's kind of random and I don't like surprise patches. But I'm open to other approaches especially if it's what your team prefers. My only goal was to reduce the logs in CI.

I don't think the warning should always be suppressed because it could indicate an issue. But as long as the PDF itself is checked against a good copy I think it's okay to suppress in a test environment. I will update the PR with your approach.

[nihil2501]

@nihil2501 I thought it might be better to keep this more visible since it's kind of random and I don't like surprise patches. But I'm open to other approaches especially if it's what your team prefers. My only goal was to reduce the logs in CI.

I don't think the warning should always be suppressed because it could indicate an issue. But as long as the PDF itself is checked against a good copy I think it's okay to suppress in a test environment. I will update the PR with your approach.

@stevenjcumming How about using refinements in just those files to apply the patch explicitly locally rather than unexplicitly globally? It'd be the first time I've ever used Ruby refinements personally.

module CombinePDF
  class PDFParser
    module WithWarningSuppressed
      MESSAGE = "Couldn't connect reference for"

      if ENV['CI'] == 'true'
        refine module_parent do
          def warn(*messages, **)
            messages.reject! { _1.start_with? MESSAGE }
            super
          end
        end
      end
    end
  end
end

parser = CombinePDF::PDFParser.new('')
parser.send(:warn, "Couldn't connect reference for", "hi")
# => Couldn't connect reference for
# => hi

using CombinePDF::PDFParser::WithWarningSuppressed
parser.send(:warn, "Couldn't connect reference for", "hi")
# => hi

[va-vsp-bot]

Error: A file (or its parent directories) does not have a CODEOWNERS entry. Please update the .github/CODEOWNERS file and add the entry for the Offending file: config/initializers/combine_pdf_log_patch.rb

[stevenjcumming]

@stevenjcumming How about using refinements in just those files to apply the patch explicitly locally rather than unexplicitly globally? It'd be the first time I've ever used Ruby refinements personally.

I think it's fine considering the potential impact is low. If someone wants to change it in the future that's fine with me.

[ericboehs]

I think we should patch this everywhere, not just CI/test. A large cost in DD is superfluous logs.

Is there a reason we should keep this in other places besides test? Perhaps we'd want it in dev but not test/prod (e.g. unless Rails.env.development?)