Skip to content

Conversation

xuan-cao-swi
Copy link
Contributor

@xuan-cao-swi xuan-cao-swi commented Jul 25, 2025

Description

  1. remove *.gemfile and rely on single Gemfile for testing
  2. instead of using bundle exec rake test, change to execute test file individually to avoid shared state among test case (e.g. some global variable OpenTelemetry.* will cause issue due to non-deterministic test case execute sequence)
  3. cleanup some leftover helper function for non-otlp apm-ruby

Test (if applicable)

@xuan-cao-swi xuan-cao-swi marked this pull request as ready for review July 25, 2025 17:25
@xuan-cao-swi xuan-cao-swi requested a review from a team as a code owner July 25, 2025 17:25
Copy link
Contributor

@cheempz cheempz left a comment

Choose a reason for hiding this comment

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

Awesome revamp @xuan-cao-swi! Left a few minor comments.

end

it 'default_test_solarwinds_ready' do
skip if ENV['APM_RUBY_TEST_STAGING_KEY'].to_s.empty?
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be an Error instead of skip when required env var for running test is not set--if we somehow accidentally remove this GH Action secret we'd never notice. In upcoming update of CONTRIB docs we should note the required env vars when running tests locally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, but in this case, when people try to run bundle exec rake docker_tests from rake, they need to provide this env like bundle exec rake docker_tests -e APM_RUBY_TEST_STAGING_KEY=....

Copy link
Contributor

Choose a reason for hiding this comment

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

that seems acceptable to me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the rake test

BUNDLE_GEMFILE=gemfiles/unit.gemfile bundle exec ruby -I test test/opentelemetry/solarwinds_propagator_test.rb -n /trace_state_header/
bundle update
bundle exec ruby -I test test/opentelemetry/solarwinds_propagator_test.rb
bundle exec ruby -I test test/opentelemetry/solarwinds_propagator_test.rb -n /trace_state_header/
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh nice you're already updating CONTRIB. How about noting the required env vars when running tests locally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

echo "--- SUMMARY ------------------------------"
grep -E '===|failures|FAIL|ERROR' "$TEST_RUNS_FILE_NAME"
echo "==================== FINAL SUMMARY ===================="
echo "Total: $total_tests | Passed: $passed_tests | Failed: $failed_tests"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
echo "Total: $total_tests | Passed: $passed_tests | Failed: $failed_tests"
echo "Total Files: $total_tests | Passed: $passed_tests | Failed: $failed_tests"

I think this "total_tests" is actually tracking the total test files (not actual test cases) that were run.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

@xuan-cao-swi xuan-cao-swi requested a review from cheempz July 28, 2025 16:31
Copy link
Contributor

@cheempz cheempz left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the revisit @xuan-cao-swi!

@xuan-cao-swi xuan-cao-swi merged commit 351fcab into main Jul 30, 2025
14 checks passed
@xuan-cao-swi xuan-cao-swi deleted the fix-test-case branch July 30, 2025 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants