Skip to content

Use new test method to avoid shared state among test case#208

Merged
xuan-cao-swi merged 5 commits into
mainfrom
fix-test-case
Jul 30, 2025
Merged

Use new test method to avoid shared state among test case#208
xuan-cao-swi merged 5 commits into
mainfrom
fix-test-case

Conversation

@xuan-cao-swi
Copy link
Copy Markdown
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
Copy Markdown
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.

Comment thread test/api/tracing_ready_test.rb Outdated
end

it 'default_test_solarwinds_ready' do
skip if ENV['APM_RUBY_TEST_STAGING_KEY'].to_s.empty?
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

Comment thread CONTRIBUTING.md
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
Copy Markdown
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
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated

Comment thread test/run_tests.sh Outdated
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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