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 test for the help command #418

Merged
merged 5 commits into from
Oct 26, 2022
Merged

Conversation

st0012
Copy link
Member

@st0012 st0012 commented Oct 24, 2022

This PR adds tests for 2 of 3 possible help command outcomes:

  • When executed without rdoc: it returns the command constant directly
    • It probably should return some useful message, but that's for another day.
  • When executed with argument, like help String#gsub: it renders the argument's document via rdoc
  • When executed without argument: it starts rdoc's interactive mode, which intercepts stdin and is not testable under the current test framework

Because the help command redefines itself based on if rdoc is required succesfully, there's no easy way to recover it to the pre-test state. I think the best way is to reload the command file, but any alternative is welcome.


::Kernel.define_method(:require) do |name|
raise LoadError, "cannot load such file -- rdoc (test)" if name == "rdoc"
original_require(name)
Copy link
Member Author

Choose a reason for hiding this comment

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

Here's a typo, but in these tests it's never reached 🙈

::Kernel.send(:alias_method, :old_require, :require)

::Kernel.define_method(:require) do |name|
raise LoadError, "cannot load such file -- rdoc (test)" if name.match?("rdoc") || name.match?(/^rdoc\/.*/)
Copy link
Member Author

Choose a reason for hiding this comment

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

I expanded it to match rdoc or rdoc/*.

@st0012
Copy link
Member Author

st0012 commented Oct 24, 2022

@junaruga I think different rdoc invocation is likely have its own quirk when being tested, I decided to deal with them in different PRs.

@st0012
Copy link
Member Author

st0012 commented Oct 25, 2022

cc @peterzhu2118

@junaruga
Copy link
Member

I think different rdoc invocation is likely have its own quirk when being tested, I decided to deal with them in different PRs.

OK. Thanks for the work!

test/lib/helper.rb Outdated Show resolved Hide resolved
test/irb/test_cmd.rb Outdated Show resolved Hide resolved
test/irb/test_cmd.rb Outdated Show resolved Hide resolved
test/irb/test_cmd.rb Outdated Show resolved Hide resolved
Co-authored-by: Peter Zhu <peter@peterzhu.ca>
@st0012 st0012 force-pushed the add-test-for-help branch from 38c4602 to a87ea90 Compare October 25, 2022 19:45
@st0012 st0012 force-pushed the add-test-for-help branch from a87ea90 to cc6e6d2 Compare October 25, 2022 19:47
"help 'String#gsub'\n",
"\n",
])
IRB.conf[:PROMPT_MODE] = :SIMPLE
Copy link
Member Author

@st0012 st0012 Oct 25, 2022

Choose a reason for hiding this comment

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

@peterzhu2118 This one is needed to reduce noise from the output.

@st0012 st0012 requested a review from peterzhu2118 October 25, 2022 19:48
@st0012 st0012 force-pushed the add-test-for-help branch from ec9440e to df4536e Compare October 26, 2022 14:07
test/irb/test_helper.rb Outdated Show resolved Hide resolved
Ruby CI runs irb and other Ruby core/stdlib tests in the same process.
So adding irb-specific helper to Test::Unit::TestCase could potentially
pollute other components' tests and should be avoided.
@st0012 st0012 force-pushed the add-test-for-help branch from df4536e to e08c981 Compare October 26, 2022 14:15
Copy link
Member

@peterzhu2118 peterzhu2118 left a comment

Choose a reason for hiding this comment

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

Looks good, thank you!

@peterzhu2118 peterzhu2118 merged commit c72fa9a into ruby:master Oct 26, 2022
@st0012 st0012 deleted the add-test-for-help branch October 26, 2022 14:18
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.

3 participants