feat: Update instrumentation generator

[AkhigbeEromo]

Updated the script to reflect the current files that need to be updated.
Updated the order of the methods generated in the instrumentation file
first draft of #978

[kaylareopelle]

@AkhigbeEromo, thank you for opening this PR! Amazing start! 😄

I think this does a good job of dynamically updating the CI file and adding tests.

However, there's a little more to do to meet the changes in the PR description.

For Updated the script to reflect the current files that need to be updated., the bin/instrumentation_generator output should probably be removed. If we're going to add the gem to the CI file dynamically, then that warning isn't needed.

For Updated the order of the methods generated in the instrumentation file, I think the instrumentation.rb.tt template would need to get updated to add the calls to compatible and option. Here's a good example on the order of the methods and how they can be called: pg/instrumentation.rb

Thanks again for your help!

[kaylareopelle]

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 evaluate¹:

• 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 ↩

[kaylareopelle]

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

[kaylareopelle]

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?

[github-actions]

👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the keep label to hold stale off permanently, or do nothing. If you do nothing this pull request will be closed eventually by the stale bot

[github-actions]

👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the keep label to hold stale off permanently, or do nothing. If you do nothing this pull request will be closed eventually by the stale bot