Fix method scope in test in order to invoke the tests properly and fix exception message #182
Conversation
The `test_empty` and `test_linear_performance_gt` were defined as private method. Seems that test-unit runner does not invoke private methods even if the methods have `test_` prefix.
| assert_equal(<<-DETAIL.chomp, exception.to_s) | ||
| Malformed notation declaration: name is missing | ||
| Line: 5 | ||
| Position: 72 | ||
| Last 80 unconsumed characters: | ||
| <!ENTITY> ]> <r/> | ||
| DETAIL |
There was a problem hiding this comment.
This test raises following exception:
<"#<NoMethodError: undefined method `captures' for nil>\n" +
"/home/watson/prj/rexml/lib/rexml/parsers/baseparser.rb:321:in `pull_event'\n" +
"/home/watson/prj/rexml/lib/rexml/parsers/baseparser.rb:225:in `pull'\n" +
"/home/watson/prj/rexml/lib/rexml/parsers/treeparser.rb:22:in `parse'\n" +
"/home/watson/prj/rexml/lib/rexml/document.rb:448:in `build'\n" +
I removed because I didn't think the need to check NoMethodError message.
There was a problem hiding this comment.
We should raise ParseException.
Could you fix the implementation in this PR?
There was a problem hiding this comment.
Ah, I was wrong above comment. sorry.
It raises ParseException, but it has #<NoMethodError: undefined method captures' for nil>` exception message.
#<REXML::ParseException:"#<NoMethodError: undefined method `captures' for nil>
There was a problem hiding this comment.
OK. Then, could you fix the NoMethodError error message in this PR?
There was a problem hiding this comment.
I fixed with 93ec28f
| assert_equal(<<-DETAIL.chomp, exception.to_s) | ||
| Malformed notation declaration: name is missing | ||
| Line: 5 | ||
| Position: 72 | ||
| Last 80 unconsumed characters: | ||
| <!ENTITY> ]> <r/> | ||
| DETAIL |
There was a problem hiding this comment.
We should raise ParseException.
Could you fix the implementation in this PR?
| end | ||
|
|
||
| def test_linear_performance_gt | ||
| seq = [10000, 50000, 100000, 150000, 200000] | ||
| assert_linear_performance(seq, rehearsal: 10) do |n| | ||
| REXML::Document.new('<!DOCTYPE rubynet [<!ENTITY rbconfig.ruby_version "' + '>' * n + '">') | ||
| begin | ||
| REXML::Document.new('<!DOCTYPE rubynet [<!ENTITY rbconfig.ruby_version "' + '>' * n + '">') |
There was a problem hiding this comment.
Can we use a valid XML to reproduce this?
There was a problem hiding this comment.
I fixed with 36ba79e
lib/rexml/parsers/baseparser.rb
Outdated
| @@ -318,7 +318,11 @@ def pull_event | |||
| raise REXML::ParseException.new( "Bad ELEMENT declaration!", @source ) if md.nil? | |||
| return [ :elementdecl, "<!ELEMENT" + md[1] ] | |||
| elsif @source.match("ENTITY", true) | |||
| match = [:entitydecl, *@source.match(Private::ENTITYDECL_PATTERN, true, term: Private::ENTITY_TERM).captures.compact] | |||
| scanner = @source.match(Private::ENTITYDECL_PATTERN, true, term: Private::ENTITY_TERM) | |||
There was a problem hiding this comment.
Could you use match_data not scanner?
We want to use MatchData instead of StringScanner because we don't want to use StringScanner in REXML::Source directly. But REXML::Source#match returns StringScanner directly for performance reason.
So we should assume that the return value of REXML::Source#match is a MatchData and we should only use MatchData compatible API provided by StringScanner.
There was a problem hiding this comment.
I see, so that's the intention.
563bb57
| Last 80 unconsumed characters: | ||
| <!ENTITY> ]> <r/> | ||
| > ]> <r/> |
There was a problem hiding this comment.
[nits] There is a unnecessary white space.
| > ]> <r/> | |
| > ]> <r/> |
There was a problem hiding this comment.
Unfortunately, the exception message contains trailing space.
So I cannot remove it.
There was a problem hiding this comment.
Ahh I see. I'm so sorry mm
|
Could you update the description that this PR also includes |
Watson1978
changed the title
Watson1978
changed the title
@kou OK, I updated the description. Thanks. |
|
Thanks. |
This PR includes following two fixes.
test_emptyandtest_linear_performance_gtwere defined as private method. Seems that test-unit runner does not invoke private methods even if the methods havetest_prefix.NoMethodError. The proper exception message will be contained by this fix.