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

Breaking change from 9.2.2 to 9.3.0 due to /test/agent_helper.rb no longer being included in the gem #2113

Closed
ajesler opened this issue Jul 5, 2023 · 4 comments · Fixed by #2115
Assignees
Labels
bug community To tag external issues and PRs submitted by the community

Comments

@ajesler
Copy link
Contributor

ajesler commented Jul 5, 2023

Description

After updating from newrelic_rpm 9.2.2 to 9.3.0, some of our specs started failing.

#2089 prevents any file in /test being shipped. Previously, there was an exclusion to allow /test/agent_helper.rb to be shipped with the newrelic_rpm gem.

See https://github.com/newrelic/newrelic-ruby-agent/blob/dev/CHANGELOG.md#v440, search for require_test_helper .

This is a breaking change to anyone who uses NewRelic::Agent.require_test_helper, implemented at https://github.com/newrelic/newrelic-ruby-agent/blob/dev/lib/new_relic/agent.rb#L476

I'll have a go at coming up with a PR that keeps the new behaviour of the .build_ignore file but allows the /test/agent_helper.rb to be included.

Expected Behavior

Using NewRelic::Agent.require_test_helper in my tests with the newrelic_rpm gem as a dependency should load the test agent helper.

Steps to Reproduce

# save this to a file reproduction.rb, and run with `ruby reproduction.rb`
#!/usr/bin/env ruby

require 'bundler/inline'

gemfile true do
  source 'https://rubygems.org'
  gem 'newrelic_rpm', '9.3.0'
end

NewRelic::Agent.require_test_helper

This will generate an output like

Fetching gem metadata from https://rubygems.org/.
Resolving dependencies...
Using bundler 2.3.24
Using newrelic_rpm 9.3.0
/Users/me/.rbenv/versions/3.1.4/lib/ruby/gems/3.1.0/gems/newrelic_rpm-9.3.0/lib/new_relic/agent.rb:478:in `require': cannot load such file -- /Users/me/.rbenv/versions/3.1.4/lib/ruby/gems/3.1.0/gems/newrelic_rpm-9.3.0/test/agent_helper (LoadError)
        from /Users/me/.rbenv/versions/3.1.4/lib/ruby/gems/3.1.0/gems/newrelic_rpm-9.3.0/lib/new_relic/agent.rb:478:in `require_test_helper'
        from newrelic_rpm_require_test_helper.rb:10:in `<main>'

Your Environment

Running ruby 3.1.4, rails 7.0.5.1 and newrelic_rpm 9.3.0

@ajesler ajesler added the bug label Jul 5, 2023
@workato-integration
Copy link

@github-actions github-actions bot added the community To tag external issues and PRs submitted by the community label Jul 5, 2023
@kaylareopelle kaylareopelle self-assigned this Jul 5, 2023
@kaylareopelle
Copy link
Contributor

Hi @ajesler, thank you for this wonderfully written bug report. I apologize for our oversight with the file distribution changes made in 9.3.0. We didn't intend to make any changes to the NewRelic::Agent.require_test_helper API.

One of the things I love about coding is that there are many ways to solve the same problem.

If you've already started your PR for the change you described, please continue it and submit it when ready.

As a public API is broken, I'd like to get a change out the door quickly. I have a draft PR with one potential solution. If you'd like to test it out for yourself or use it instead of the solution you're working on, please set your newrelic_rpm installation in your Gemfile to:

gem 'newrelic_rpm', git: 'https://github.com/newrelic/newrelic-ruby-agent', branch: 'bugfix/2113-include-test-agent-helper'

@ajesler
Copy link
Contributor Author

ajesler commented Jul 6, 2023

Thanks for the fast response @kaylareopelle. I updated the Gemfile to use your newrelic_rpm branch, and the our tests pass again 🎉

I was trying to figure out how to write a test for this, but an elegant way is so far eluding me. I need to do some more research on gemspecs. But no point waiting for that when you have a working PR.

This was referenced Jul 6, 2023
@kaylareopelle
Copy link
Contributor

I'm so glad to hear the branch is working! 🌱

Test writing for this stumped me too. If you come up with something, I'd love to hear it! I've created a separate issue to add testing to prevent a regression: #2117

I'm running solo this week while the company is on "Relic Recharge Week", but we should be able to get this merged and released once my colleagues return. In the meantime, I'll keep the branch open and stable until it's part of a release.

Thanks again for your help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug community To tag external issues and PRs submitted by the community
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants