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.

Sign up for GitHub

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

feat: Update instrumentation generator #1094

Closed
wants to merge 3 commits into from
Closed

feat: Update instrumentation generator #1094

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ac75f31
First draft of PR
AkhigbeEromo Jul 31, 2024
eb1fb05
Merge branch 'main' into Update-instrumentation-generator
AkhigbeEromo Jul 31, 2024
04c1112
Update ci-instrumentation.yml
AkhigbeEromo Aug 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/ci-instrumentation.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ jobs:
- restclient
- rspec
- sinatra
- <instrumentation_name>
AkhigbeEromo marked this conversation as resolved.
Show resolved Hide resolved
os:
- ubuntu-latest

Expand Down
13 changes: 13 additions & 0 deletions .instrumentation_generator/instrumentation_generator.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,19 @@ def add_to_instrumentation_all
insert_into_file("#{instrumentation_all_path}/lib/opentelemetry/instrumentation/all.rb", all_rb_text, after: "# SPDX-License-Identifier: Apache-2.0\n")
end

def update_ci_workflow
ci_file = '.github/workflows/ci-instrumentation.yml'
dependabot_file = '.github/dependabot.yml'

# Update the CI workflow file
insert_into_file(ci_file, " - #{instrumentation_name}\n", after: " gem:\n")

# Update the Dependabot configuration file
insert_into_file(dependabot_file, " - package-ecosystem: 'bundler'\n directory: '/instrumentation/#{instrumentation_name}'\n", after: "updates:\n")

puts "Updated #{ci_file} and #{dependabot_file} successfully."
end

private

def opentelemetry_version
Expand Down
84 changes: 84 additions & 0 deletions .instrumentation_generator/test/instrumentation_generator_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
# test/instrumentation_generator_test.rb

require 'minitest/autorun'
require 'mocha/minitest'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't able to run these tests locally. When I tried to run the file from the root directory using:

bundle exec ruby .instrumentation_generator/test/instrumentation_generator_test.rb

I got the following error:

/Users/kreopelle/.rbenv/versions/3.3.1/lib/ruby/gems/3.3.0/gems/mocha-1.16.1/lib/mocha/integration/mini_test/adapter.rb:26:in `included': uninitialized constant MiniTest (NameError)

          Mocha::ExpectationErrorFactory.exception_class = ::MiniTest::Assertion
                                                           ^^^^^^^^^^
Did you mean?  Minitest
        from /Users/kreopelle/.rbenv/versions/3.3.1/lib/ruby/gems/3.3.0/gems/mocha-1.16.1/lib/mocha/integration/mini_test.rb:50:in `include'
        from /Users/kreopelle/.rbenv/versions/3.3.1/lib/ruby/gems/3.3.0/gems/mocha-1.16.1/lib/mocha/integration/mini_test.rb:50:in `activate'
        from /Users/kreopelle/.rbenv/versions/3.3.1/lib/ruby/gems/3.3.0/gems/mocha-1.16.1/lib/mocha/minitest.rb:5:in `<top (required)>'
        from <internal:/Users/kreopelle/.rbenv/versions/3.3.1/lib/ruby/site_ruby/3.3.0/rubygems/core_ext/kernel_require.rb>:37:in `require'
        from <internal:/Users/kreopelle/.rbenv/versions/3.3.1/lib/ruby/site_ruby/3.3.0/rubygems/core_ext/kernel_require.rb>:37:in `require'
        from instrumentation_generator_test.rb:4:in `<main>'

It looks like mocha hasn't updated its call to MiniTest::Assertion to use the Minitest spelling. Minitest used to accept both spellings, but now it only accepts Minitest.

Minitest has its own stubbing features that can be used, though you have to use them in a specific test and can't set them in a setup method. Here's one of my favorite articles on stubbing in Minitest: https://semaphoreci.com/community/tutorials/mocking-in-ruby-with-minitest

When I removed mocha/minitest and took the @generator.stubs method calls out of the setup method, I was able to run the tests.

The only error I got was related to calling puts:

  1) Error:
InstrumentationGeneratorTest#test_update_ci_workflow:
NoMethodError: private method `puts' called for an instance of InstrumentationGenerator
    .instrumentation_generator/test/instrumentation_generator_test.rb:86:in `test_update_ci_workflow'

6 runs, 21 assertions, 0 failures, 1 errors, 0 skips

require 'thor'
require_relative '../instrumentation_generator'

class InstrumentationGeneratorTest < Minitest::Test
def setup
@instrumentation_name = 'new_instrumentation'
@generator = InstrumentationGenerator.new([@instrumentation_name])

@generator.stubs(:template)
@generator.stubs(:insert_into_file)
@generator.stubs(:puts)
end
Comment on lines +9 to +16
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since running the generator creates files, I think the files should be deleted between the tests to make sure we have a clean slate for each test.

Like the setup method you added, minitest also has a teardown method.

Maybe in teardown we could delete the directory we expect to get created if it exists?


def test_root_files
@generator.root_files

assert @generator.template('templates/rubocop.yml.tt', 'instrumentation/new_instrumentation/.rubocop.yml')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First off, thank you for adding tests! It's great to have coverage on a tool that we use whenever we add new instrumentation.

I like this example as the three options a Ruby unit test should evaluate1:

  • It returns a value.
  • It passes the work along to somewhere else (i.e., dispatches the work elsewhere).
  • It causes a side effect.

In this test, it's almost as if we're testing whether the @generator.template method can be called. Since we don't define @generator.template, (that's from Thor), I think we could take a different approach to make sure everything is working as expected.

Of the three options listed above, I think we want to make sure this method causes a side effect.

Since the root_files method is supposed to create these files from the ERB templates, what if we checked to see if the files were created when the method is run?

That could look like:
assert File.exist?('instrumentation/new_instrumentation/.yardopts')

Let me know if you want help brainstorming assertions related to side effects of the other methods.

Footnotes

  1. https://www.cloudbees.com/blog/unit-testing-in-ruby#what-should-be-tested ↩

assert @generator.template('templates/yardopts.tt', 'instrumentation/new_instrumentation/.yardopts')
assert @generator.template('templates/Appraisals', 'instrumentation/new_instrumentation/Appraisals')
assert @generator.template('templates/CHANGELOG.md.tt', 'instrumentation/new_instrumentation/CHANGELOG.md')
assert @generator.template('templates/Gemfile', 'instrumentation/new_instrumentation/Gemfile')
assert @generator.template('templates/LICENSE', 'instrumentation/new_instrumentation/LICENSE')
assert @generator.template('templates/gemspec.tt', 'instrumentation/new_instrumentation/opentelemetry-instrumentation-new_instrumentation.gemspec')
assert @generator.template('templates/Rakefile', 'instrumentation/new_instrumentation/Rakefile')
assert @generator.template('templates/Readme.md.tt', 'instrumentation/new_instrumentation/README.md')
end

def test_lib_files
@generator.lib_files

assert @generator.template('templates/lib/entrypoint.rb', 'instrumentation/new_instrumentation/lib/opentelemetry-instrumentation-new_instrumentation.rb')
assert @generator.template('templates/lib/instrumentation.rb.tt', 'instrumentation/new_instrumentation/lib/opentelemetry/instrumentation.rb')
assert @generator.template('templates/lib/instrumentation/instrumentation_name.rb.tt', 'instrumentation/new_instrumentation/lib/opentelemetry/instrumentation/new_instrumentation.rb')
assert @generator.template('templates/lib/instrumentation/instrumentation_name/instrumentation.rb.tt', 'instrumentation/new_instrumentation/lib/opentelemetry/instrumentation/new_instrumentation/instrumentation.rb')
assert @generator.template('templates/lib/instrumentation/instrumentation_name/version.rb.tt', 'instrumentation/new_instrumentation/lib/opentelemetry/instrumentation/new_instrumentation/version.rb')
end

def test_test_files
@generator.test_files

assert @generator.template('templates/test/test_helper.rb', 'instrumentation/new_instrumentation/test/test_helper.rb')
assert @generator.template('templates/test/instrumentation.rb', 'instrumentation/new_instrumentation/test/opentelemetry/instrumentation/new_instrumentation/instrumentation_test.rb')
end

def test_add_to_releases
@generator.add_to_releases

expected_release_details = <<-HEREDOC
- name: opentelemetry-instrumentation-new_instrumentation
directory: instrumentation/new_instrumentation
version_constant: [OpenTelemetry, Instrumentation, NewInstrumentation, VERSION]\n
HEREDOC

assert @generator.insert_into_file('.toys/.data/releases.yml', expected_release_details.strip, after: "gems:\n")
end

def test_add_to_instrumentation_all
@generator.add_to_instrumentation_all

gemfile_text = "\ngem 'opentelemetry-instrumentation-new_instrumentation', path: '../new_instrumentation'"
gemspec_text = "\n spec.add_dependency 'opentelemetry-instrumentation-new_instrumentation', '~> 0.0.0'"
all_rb_text = "\nrequire 'opentelemetry-instrumentation-new_instrumentation'"

assert @generator.insert_into_file('instrumentation/all/Gemfile', gemfile_text, after: "gemspec\n")
assert @generator.insert_into_file('instrumentation/all/opentelemetry-instrumentation-all.gemspec', gemspec_text, after: "spec.required_ruby_version = '>= 3.0'\n")
assert @generator.insert_into_file('instrumentation/all/lib/opentelemetry/instrumentation/all.rb', all_rb_text, after: "# SPDX-License-Identifier: Apache-2.0\n")
end

def test_update_ci_workflow
@generator.update_ci_workflow

ci_file_text = " - new_instrumentation\n"
dependabot_file_text = " - package-ecosystem: 'bundler'\n directory: '/instrumentation/new_instrumentation'\n"

assert @generator.insert_into_file('.github/workflows/ci-instrumentation.yml', ci_file_text, after: " gem:\n")
assert @generator.insert_into_file('.github/dependabot.yml', dependabot_file_text, after: "updates:\n")

assert @generator.puts('Updated .github/workflows/ci-instrumentation.yml and .github/dependabot.yml successfully.')
end
end
2 changes: 2 additions & 0 deletions Gemfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,5 @@ source 'https://rubygems.org'
gem 'rake', '~> 13.0'
gem 'rubocop', '~> 1.64.0'
gem 'rubocop-performance', '~> 1.21.0'
gem 'minitest', '~> 5.14'
gem 'mocha', '~> 1.12', require: false
Loading