Skip to content

Some tests#3

Merged
hsbt merged 11 commits intoruby:masterfrom
BurdetteLamar:test
Oct 26, 2021
Merged

Some tests#3
hsbt merged 11 commits intoruby:masterfrom
BurdetteLamar:test

Conversation

@BurdetteLamar
Copy link
Copy Markdown
Member

I started out to enhance the RDoc for GetoptLong. There were some things I did not understand, so I looked for the tests, which are often clarifying. There were none. So here are some. More to come.

@BurdetteLamar
Copy link
Copy Markdown
Member Author

@hsbt, I have switched over to test-unit. However, I did not find a substitute for capture_subprocess_io, so have commented out the offending test method test_required_argument_missing.

@BurdetteLamar BurdetteLamar marked this pull request as draft October 18, 2021 12:22
@BurdetteLamar BurdetteLamar marked this pull request as ready for review October 18, 2021 21:33
@BurdetteLamar BurdetteLamar requested a review from hsbt October 18, 2021 21:33
@hsbt
Copy link
Copy Markdown
Member

hsbt commented Oct 18, 2021

You should rename test/test.rb because Ruby's test file expect test prefix or suffix. This file is confusing to us.

@BurdetteLamar
Copy link
Copy Markdown
Member Author

BurdetteLamar commented Oct 18, 2021

You should rename test/test.rb because Ruby's test file expect test prefix or suffix. This file is confusing to us.

I quite agree, but I could not think of a good name: dummy.rb? target.rb? program.rb? I welcome suggestions.

For now, I'll name it test/options.rb.

@BurdetteLamar
Copy link
Copy Markdown
Member Author

I've responded to comments, and also added some explanation of how the tests work.

@BurdetteLamar BurdetteLamar requested a review from hsbt October 18, 2021 23:33
@BurdetteLamar
Copy link
Copy Markdown
Member Author

Changed the name of the test program to t.rb.

@hsbt
Copy link
Copy Markdown
Member

hsbt commented Oct 19, 2021

This test is not working on https://github.com/ruby/ruby. Because spawning ruby depends on PATH variable. Basically, you need to support them.

@BurdetteLamar
Copy link
Copy Markdown
Member Author

This test is not working on https://github.com/ruby/ruby. Because spawning ruby depends on PATH variable. Basically, you need to support them.

Thanks @hsbt. I don't know about that. Can you give some direction? Or even make the needed changes in the PR?

@BurdetteLamar
Copy link
Copy Markdown
Member Author

I'm going to see whether I can find an entirely different test strategy.

@BurdetteLamar BurdetteLamar marked this pull request as draft October 20, 2021 01:13
@BurdetteLamar
Copy link
Copy Markdown
Member Author

I have a new strategy that does not involve either subprocess or t.rb. Please don't spend time with this until I update with new stuff.

@BurdetteLamar
Copy link
Copy Markdown
Member Author

I have changed the test strategy. Instead of using a command-line, I'm using a dummy ARGV, which greatly simplifies things.

@BurdetteLamar BurdetteLamar marked this pull request as ready for review October 20, 2021 14:06
@BurdetteLamar
Copy link
Copy Markdown
Member Author

Once we've settled on a strategy, there's much more testing that I can add. Also: amazing how much I've learned by doing these tests.

@BurdetteLamar
Copy link
Copy Markdown
Member Author

I probably should have said more about this new test strategy.

In the end, I think it is a good thing that there was a problem with spawning a process. It made me think things through and realize that GetoptLong doesn't get its input from the command line; it gets it from ARGV. So that can be our test source, not the command line.

Btw, I learned on the web that ARGV = causes a warning, while ARGV.replace does not.

@BurdetteLamar BurdetteLamar requested review from hsbt and removed request for hsbt October 21, 2021 19:01
@BurdetteLamar BurdetteLamar requested review from hsbt and removed request for hsbt October 22, 2021 13:44
@hsbt
Copy link
Copy Markdown
Member

hsbt commented Oct 22, 2021

Don't use generic file name like test/test_new.rb. At least it should be test_getoptlong.rb.

@BurdetteLamar
Copy link
Copy Markdown
Member Author

Don't use generic file name like test/test_new.rb. At least it should be test_getoptlong.rb.

Changed.

@BurdetteLamar BurdetteLamar requested a review from hsbt October 22, 2021 23:59
@BurdetteLamar BurdetteLamar requested review from hsbt and removed request for hsbt October 23, 2021 00:33
actual_options = []
opts.each do |opt, arg|
actual_options << "#{opt}: #{arg}"
end
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.

Minor: could we #map over the collection?

@olleolleolle
Copy link
Copy Markdown
Contributor

@BurdetteLamar 🎉 Looking better and better, and the "revised test strategy" seems intuitive and easy to follow.

@BurdetteLamar
Copy link
Copy Markdown
Member Author

@BurdetteLamar 🎉 Looking better and better, and the "revised test strategy" seems intuitive and easy to follow.

Thanks, @olleolleolle, for your encouragement.

@BurdetteLamar
Copy link
Copy Markdown
Member Author

@hsbt, can we get this PR reviewed, approved, and merged? I'll have much more, both RDoc and tests. Thanks.

@hsbt
Copy link
Copy Markdown
Member

hsbt commented Oct 25, 2021

👍 This test is working with ruby core repo. But I got the some of warnings like:

$ make test-all -j TESTS="test_getoptlong.rb -j12"
generating vm_call_iseq_optimized.inc
generating known_errors.inc
known_errors.inc unchanged
vm_call_iseq_optimized.inc unchanged
./revision.h unchanged
Run options:
  --seed=59269
  "--ruby=./miniruby -I./lib -I. -I.ext/common  ./tool/runruby.rb --extout=.ext  -- --disable-gems"
  --excludes-dir=./test/excludes
  --name=!/memory_leak/
  -j12

# Running tests:

/Users/hsbt/Documents/github.com/ruby/ruby/tool/lib/test/unit/parallel.rb: TestGetoptLong#test_required_argument_missing: option `--xxx' requires an argument
/Users/hsbt/Documents/github.com/ruby/ruby/tool/lib/test/unit/parallel.rb: TestGetoptLong#test_required_argument_missing: option `--xx' requires an argument
/Users/hsbt/Documents/github.com/ruby/ruby/tool/lib/test/unit/parallel.rb: TestGetoptLong#test_required_argument_missing: option `--x' requires an argument
/Users/hsbt/Documents/github.com/ruby/ruby/tool/lib/test/unit/parallel.rb: TestGetoptLong#test_required_argument_missing: option requires an argument -- x
/Users/hsbt/Documents/github.com/ruby/ruby/tool/lib/test/unit/parallel.rb: TestGetoptLong#test_required_argument_missing: option `--aaa' requires an argument
/Users/hsbt/Documents/github.com/ruby/ruby/tool/lib/test/unit/parallel.rb: TestGetoptLong#test_required_argument_missing: option `--aa' requires an argument
/Users/hsbt/Documents/github.com/ruby/ruby/tool/lib/test/unit/parallel.rb: TestGetoptLong#test_required_argument_missing: option `--a' requires an argument
/Users/hsbt/Documents/github.com/ruby/ruby/tool/lib/test/unit/parallel.rb: TestGetoptLong#test_required_argument_missing: option requires an argument -- a
Finished tests in 0.195704s, 66.4268 tests/s, 684.7075 assertions/s.
13 tests, 134 assertions, 0 failures, 0 errors, 0 skips

Can you suppress them?

@BurdetteLamar
Copy link
Copy Markdown
Member Author

Running quiet now.

@hsbt hsbt merged commit 18cdb03 into ruby:master Oct 26, 2021
@hsbt
Copy link
Copy Markdown
Member

hsbt commented Oct 26, 2021

@BurdetteLamar Merged. Additional request: You should write a descriptive commit message in the next time. I wonder why you ignore its advice.

@BurdetteLamar
Copy link
Copy Markdown
Member Author

@BurdetteLamar Merged. Additional request: You should write a descriptive commit message in the next time. I wonder why you ignore its advice.

Not sure what you mean, @hsbt. Is it that each commit should have a distinct message?

(There's lots I don't know, but I don't deliberately ignore things.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants