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

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

Related issue(s)

• https://github.com/department-of-veterans-affairs/va.gov-team/issues/78425

Testing done

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

What areas of the site does it impact?

• modules/claims_api
• modules/simple_forms_api

Acceptance Criteria

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

[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?)