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

Add specs for Thread.each_caller_location #1040

Merged

Conversation

AI-Mozi
Copy link
Contributor

@AI-Mozi AI-Mozi commented May 24, 2023

#1016
[Feature #16663]

Thread.each_caller_location is added.

@AI-Mozi
Copy link
Contributor Author

AI-Mozi commented May 24, 2023

What else is worth testing?
Also I'm not sure if these test cases meaningful enough.

@AI-Mozi AI-Mozi marked this pull request as ready for review May 24, 2023 13:06
@andrykonchin
Copy link
Member

andrykonchin commented May 24, 2023

What cases I would consider:

  • it iterates through the current execution stack
    • we can check e.g. that every yielded value is instance of Thread::Backtrace::Location
    • and that all the yielded locations equal exactly what #caller_locations returns
  • it raises LocalJumpError when called without a block
  • it doesn't accept positional and keyword arguments
  • it returns nil
  • it raises StopIteration at the end when used as an external enumerator (there is such test case already)

There are two more cases mentioned in the https://bugs.ruby-lang.org/issues/16663:

  • break stops the iteration
  • using to_enum (Thread.to_enum(:each_caller_location)) doesn't add any locations related to to_enum implementation, so locations should be the same as Thread.each_caller_location {} returns.

Maybe it also makes sense to add a similar test case:
https://github.com/ruby/spec/blob/d59824930da3522aad8d3edf0e146a4f4f595533/core/kernel/caller_spec.rb#L59C4-L69

There is a WIP PR in TruffleRuby oracle/truffleruby#3050 that contains some specs but I think this PR can be merged independently if there are more detailed test cases.

@AI-Mozi AI-Mozi force-pushed the add_specs_for_thread_each_caller_location branch from e416b01 to af20488 Compare May 26, 2023 09:01
@AI-Mozi
Copy link
Contributor Author

AI-Mozi commented May 26, 2023

Maybe it also makes sense to add a similar test case:
https://github.com/ruby/spec/blob/d59824930da3522aad8d3edf0e146a4f4f595533/core/kernel/caller_spec.rb#L59C4-L69

How would you see implementations of this test because I have no idea 😅?

core/thread/each_caller_location_spec.rb Show resolved Hide resolved
core/thread/each_caller_location_spec.rb Show resolved Hide resolved
core/thread/each_caller_location_spec.rb Show resolved Hide resolved
core/thread/each_caller_location_spec.rb Show resolved Hide resolved
core/thread/each_caller_location_spec.rb Outdated Show resolved Hide resolved
core/thread/each_caller_location_spec.rb Outdated Show resolved Hide resolved
core/thread/each_caller_location_spec.rb Outdated Show resolved Hide resolved
@AI-Mozi AI-Mozi force-pushed the add_specs_for_thread_each_caller_location branch from af20488 to 96b5b45 Compare May 26, 2023 13:48
core/thread/each_caller_location_spec.rb Outdated Show resolved Hide resolved
core/thread/each_caller_location_spec.rb Outdated Show resolved Hide resolved
core/thread/each_caller_location_spec.rb Outdated Show resolved Hide resolved
core/thread/each_caller_location_spec.rb Outdated Show resolved Hide resolved
core/thread/each_caller_location_spec.rb Show resolved Hide resolved
core/thread/each_caller_location_spec.rb Show resolved Hide resolved
@AI-Mozi AI-Mozi force-pushed the add_specs_for_thread_each_caller_location branch 2 times, most recently from 404a470 to bf25938 Compare May 30, 2023 06:16
@AI-Mozi AI-Mozi force-pushed the add_specs_for_thread_each_caller_location branch from bf25938 to f8a958f Compare May 30, 2023 10:25
@andrykonchin
Copy link
Member

Thank you!

@andrykonchin andrykonchin merged commit 93787e6 into ruby:master May 30, 2023
@AI-Mozi
Copy link
Contributor Author

AI-Mozi commented May 30, 2023

Thank you too, it was a tough one 😅

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