Add more invalid test cases for parsing entitly declaration by Watson1978 · Pull Request #183 · ruby/rexml

Watson1978 · 2024-07-19T09:28:06Z

This patch will add the test cases to verify that it raises an exception properly when parsing malformed entity declaration.

kou · 2024-07-22T00:07:49Z

+    class TestCharacterCode < self
+      def test_pubid_literal


Suggested change

class TestCharacterCode < self

def test_pubid_literal

class TestPublicIDLiteral < self

def test_invalid_pubid_char

Could you add PubidLiteral and PubidChar definitions as a comment to understand why this value is invalid?

https://www.w3.org/TR/2006/REC-xml11-20060816/#NT-PubidLiteral

https://www.w3.org/TR/2006/REC-xml11-20060816/#NT-PubidChar

kou · 2024-07-22T00:08:24Z

+    class TestCharacterCode < self
+      def test_pubid_literal
+        exception = assert_raise(REXML::ParseException) do
+          REXML::Document.new('<!DOCTYPE root [<!ENTITY foo PUBLIC "あいう" "bar" > ]>')


Could you use \U{...} instead of あ to keep this code ASCII only?

kou · 2024-07-22T00:14:12Z


    public
+
+    class TestGarbagedValue < self


Could you use syntax part name such as GeneralEntityDeclration instead of GarbgedValue for test case name? We want to organize tests based of syntax part not invalid type to avoid missing case.

Suggested change

class TestGarbagedValue < self

class TestGeneralEntityDeclration < self

# Cover all GEDecl cases in this instead of spreading multiple test cases

def test_invalid_entity_value

def test_invalid_external_id

Could you also add generic entity declaration definition (and related definitions in this test case) as a comment for easy to understand?

https://www.w3.org/TR/2006/REC-xml11-20060816/#NT-GEDecl

kou · 2024-07-22T00:15:34Z

+    class TestGarbagedValue < self
+      def test_garbaged_after_entity_definition
+        exception = assert_raise(REXML::ParseException) do
+          REXML::Document.new('<!DOCTYPE root [<!ENTITY "foo" xxx > ]>')


Could you use more easy to understand sample values instead of "foo" and xxx?
For example, "valid-name" and invalid-external-id.

Watson1978 · 2024-07-22T02:58:33Z

@kou Thank you for your review. I fixed the tests.

kou

Could you add comments that which specifications are referred in each test?

kou · 2024-07-22T05:18:23Z

+        DETAIL
+      end
+
+      class TestSystemExternalEntities < self


Suggested change

class TestSystemExternalEntities < self

class TestSystemLiteral < self

kou · 2024-07-22T05:19:16Z

+      end
+
+      class TestSystemExternalEntities < self
+        def test_invalid_external_entity_before_system_literal


Suggested change

def test_invalid_external_entity_before_system_literal

def test_no_quote

kou · 2024-07-22T05:19:29Z

+      class TestSystemExternalEntities < self
+        def test_invalid_external_entity_before_system_literal
+          exception = assert_raise(REXML::ParseException) do
+            REXML::Document.new('<!DOCTYPE root [<!ENTITY valid-name SYSTEM invalid-external-entity "valid-system-literal" > ]>')


Is "valid-system-literal" needed here?

kou · 2024-07-22T05:21:03Z

+          DETAIL
+        end
+
+        def test_invalid_external_entity_after_system_literal


Could you split this to TestNotationDataDeclaration?

Suggested change

def test_invalid_external_entity_after_system_literal

def test_not_ndata

kou · 2024-07-22T05:23:53Z

+
+        def test_invalid_external_entity_after_system_literal
+          exception = assert_raise(REXML::ParseException) do
+            REXML::Document.new('<!DOCTYPE root [<!ENTITY valid-name SYSTEM "valid-system-literal" invalid-external-entity > ]>')


not-ndata or something is better than invalid-external-entity because it should be NDATA.

kou · 2024-07-22T05:24:35Z

+        end
+      end
+
+      class TestPublicExternalEntities < self


Suggested change

class TestPublicExternalEntities < self

class TestPublicLiteral < self

kou · 2024-07-22T05:24:52Z

+Line: 1
+Position: 76
+Last 80 unconsumed characters:
+ valid-name PUBLIC \"あ\" \"valid-system-literal\" > ]>


Could you use \uXXXX?

This patch will add the test cases to verify that it raises an exception properly when parsing malformed entity declaration. Fix test class names

Watson1978 · 2024-07-23T02:02:16Z

I completely rewrote the code and added tests to cover the specification.

kou

Could you also add cases that has garbage after valid pattern?

kou · 2024-07-23T02:05:32Z

+    class TestGeneralEntityDeclaration < self
+      # https://www.w3.org/TR/2006/REC-xml11-20060816/#NT-GEDecl


Could you use a comment for class instead of a comment in class?

Suggested change

class TestGeneralEntityDeclaration < self

# https://www.w3.org/TR/2006/REC-xml11-20060816/#NT-GEDecl

# https://www.w3.org/TR/2006/REC-xml11-20060816/#NT-GEDecl

class TestGeneralEntityDeclaration < self

kou · 2024-07-23T02:05:47Z

+        class TestEntityValue < self
+          def test_no_quote
+            # https://www.w3.org/TR/2006/REC-xml11-20060816/#NT-EntityValue


Suggested change

class TestEntityValue < self

def test_no_quote

# https://www.w3.org/TR/2006/REC-xml11-20060816/#NT-EntityValue

# https://www.w3.org/TR/2006/REC-xml11-20060816/#NT-EntityValue

class TestEntityValue < self

def test_no_quote

kou · 2024-07-23T02:06:08Z

+          def test_no_quote
+            # https://www.w3.org/TR/2006/REC-xml11-20060816/#NT-EntityValue
+            exception = assert_raise(REXML::ParseException) do
+              REXML::Document.new('<!DOCTYPE root [<!ENTITY valid-name invalid-entity-value > ]>')


Could you remove needless spaces?

Suggested change

REXML::Document.new('<!DOCTYPE root [<!ENTITY valid-name invalid-entity-value > ]>')

REXML::Document.new('<!DOCTYPE root [<!ENTITY valid-name invalid-entity-value>]>')

kou · 2024-07-23T02:06:39Z

+            DETAIL
+          end
+
+          def test_invalid_entity_value


Could you use why this is an invalid value for test name instead of test_invalid_entity_value like you did for test_no_quote?

Changed the method name at 98e0d8f

kou · 2024-07-23T02:06:55Z

+            DETAIL
+          end
+
+          class TestExternalId < self


Suggested change

class TestExternalId < self

class TestExternalID < self

kou · 2024-07-23T02:09:46Z

+          end
+
+          class TestNDataDeclaration < self
+            def test_no_quote


Is "no quote" a suitable reason?
I think that NDATA is only the valid keyword.

Ah, sorry. It is my mistake.

kou · 2024-07-23T02:11:25Z

+            end
+          end
+
+          class TestNDataDeclaration < self


Could you also add a test for invalid Name in NDataDecl ::= S 'NDATA' S Name?

I added tests for Name at 9ecad7c

kou · 2024-07-23T02:11:43Z

+
+    class TestParsedEntityDeclaration < self
+      # https://www.w3.org/TR/2006/REC-xml11-20060816/#NT-PEDecl
+      class ParsedEntityDefinition < self


Suggested change

class ParsedEntityDefinition < self

class TestParsedEntityDefinition < self

kou · 2024-07-23T02:12:19Z

+        class TestEntityValue < self
+          class TestEntityValue < self


Can we reduce a needless class?

Ah, sorry. It is my mistake.

Fixed at 77b23ae

kou · 2024-07-23T02:13:40Z

+      def test_unnecessary_ndata_declaration
+        # https://www.w3.org/TR/2006/REC-xml11-20060816/#NT-PEDef
+        exception = assert_raise(REXML::ParseException) do
+          REXML::Document.new('<!DOCTYPE root [<!ENTITY % valid-name "valid-entity-value" "NDATA" valid-ndata-value > ]>')


Suggested change

REXML::Document.new('<!DOCTYPE root [<!ENTITY % valid-name "valid-entity-value" "NDATA" valid-ndata-value > ]>')

REXML::Document.new('<!DOCTYPE root [<!ENTITY % valid-name "valid-entity-value" NDATA valid-ndata-value > ]>')

BTW, should we use the ExternalID NDataDecl? part instead of the EntityValue part in EntityDef ::= EntityValue | ExternalID NDataDecl?)? If we want to use the EntityValue part, we should use "garbage" or something instead of ndata_declaration because all contents (including NDataDecl) are invalid after the EntityValue.

Make sence.
We should make sure to check that any unnecessary NDataDecl after EntityValue will raise an exception.

Added test at 4782084

otegami

Thank you for adding test cases which follows XML's entity declarations.

https://www.w3.org/TR/2006/REC-xml11-20060816/#sec-entity-decl

Should we test PEReference or Reference which are included in EntityValue although those test cases might be out of scope ?
And also, how about adding the following test cases?

Using single quotes
Using mixed quotes (' dummy " or " dummy ')
Removing necessary spaces

kou · 2024-07-23T04:45:57Z

+            DETAIL
+          end
+
+          def test_unnecessary_ndata_declaration


Could you move this to TestEntityDefinition and use test_entity_value_and_notation_data_declaration?

This test case is for EntityValue not EntityDef.

Fixed at c211ab5

Watson1978 · 2024-07-23T05:43:08Z

* Using single quotes

I want to add invalid cases in this PR. I think this case is valid test.
So, I didn't add this case.

* Using mixed quotes (`' dummy "` or `" dummy '`)

* Removing necessary spaces

I added at

Thanks

otegami

Thanks for implementing it. Although I made a few comments, it looks good overall !

otegami · 2024-07-23T07:10:52Z

+        # https://www.w3.org/TR/2006/REC-xml11-20060816/#NT-ExternalID
+        class TestExternalID < self
+          # https://www.w3.org/TR/2006/REC-xml11-20060816/#NT-SystemLiteral
+          class TestSystemLiteral < self


How about adding the cases that there is no SystemLiteral ?

Thanks. I added the test cases at 8b42c31

Co-authored-by: takuya kodama <a.s.takuya1026@gmail.com>

otegami

It looks nice to me, though I left a small comment 👍🏾

kou · 2024-07-23T21:30:51Z

+
+          def test_mixed_quote
+            exception = assert_raise(REXML::ParseException) do
+              REXML::Document.new('<!DOCTYPE root [<!ENTITY valid-name "invalid-entity-value\'>]>')


Could you use " for all string literals to avoid the same problem as #166 ?

Fixed at 556487b

kou · 2024-07-23T21:32:31Z

+Line: 1
+Position: 61
+Last 80 unconsumed characters:
+ valid-name \"invalid-entity-value'>]>


Do we need \ before " in here document?

The escapes are unnecessary.
I removed them at 4bada10

Co-authored-by: takuya kodama <a.s.takuya1026@gmail.com>

otegami

LGTM 👍🏾

kou · 2024-07-24T03:51:10Z

Thanks.

sebm123 · 2024-07-30T14:09:21Z

This patch will add the test cases to verify that it raises an exception properly when parsing malformed entity declaration.

kou reviewed Jul 22, 2024

View reviewed changes

Add more invalid test cases for parsing entitly declaration

8881bd5

This patch will add the test cases to verify that it raises an exception properly when parsing malformed entity declaration. Fix test class names

Watson1978 force-pushed the add-tests branch from edc80f8 to 8881bd5 Compare July 23, 2024 02:01

kou reviewed Jul 23, 2024

View reviewed changes

Watson1978 added 3 commits July 23, 2024 11:33

Remove redundant test class

77b23ae

Fix comment position

b01f4c4

Remove useless spaces

7047bd8

otegami reviewed Jul 23, 2024

View reviewed changes

Watson1978 added 6 commits July 23, 2024 11:53

Fix test name

98e0d8f

Fix class name

daaf615

Remove unnecessary quote around NDATA

aa634ff

Add test for Name

9ecad7c

Fix nest of test class

7c1c59d

Add test for unnecessary ndata declaration

4782084

kou reviewed Jul 23, 2024

View reviewed changes

Watson1978 added 4 commits July 23, 2024 14:00

Remove redundant test

f7d4a08

Adjust the position of test and rename the method

c211ab5

Add mixed quote tests

635af12

Add test for spaces

96c1d9c

otegami reviewed Jul 23, 2024

View reviewed changes

Watson1978 and others added 2 commits July 23, 2024 18:43

Fix test name

7e95837

Co-authored-by: takuya kodama <a.s.takuya1026@gmail.com>

Add tests for no literal case

8b42c31

otegami reviewed Jul 23, 2024

View reviewed changes

Comment thread test/parse/test_entity_declaration.rb Outdated

kou reviewed Jul 23, 2024

View reviewed changes

Watson1978 and others added 2 commits July 24, 2024 09:39

Fix test name

8b245cb

Co-authored-by: takuya kodama <a.s.takuya1026@gmail.com>

Replace " to '

556487b

Remove unnecessary escape in here document

4bada10

otegami reviewed Jul 24, 2024

View reviewed changes

kou merged commit 086287c into ruby:master Jul 24, 2024

Watson1978 deleted the add-tests branch July 24, 2024 08:01

-    class TestGarbagedValue < self
+    class TestGeneralEntityDeclration < self
+      # Cover all GEDecl cases in this instead of spreading multiple test cases
+      def test_invalid_entity_value
+      def test_invalid_external_id

	class TestSystemExternalEntities < self
	class TestSystemLiteral < self

	def test_invalid_external_entity_before_system_literal
	def test_no_quote

	def test_invalid_external_entity_after_system_literal
	def test_not_ndata

	class TestPublicExternalEntities < self
	class TestPublicLiteral < self

		class TestGeneralEntityDeclaration < self
		# https://www.w3.org/TR/2006/REC-xml11-20060816/#NT-GEDecl

	REXML::Document.new('<!DOCTYPE root [<!ENTITY valid-name invalid-entity-value > ]>')
	REXML::Document.new('<!DOCTYPE root [<!ENTITY valid-name invalid-entity-value>]>')

	class ParsedEntityDefinition < self
	class TestParsedEntityDefinition < self

Conversation

Watson1978 commented Jul 19, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Watson1978 commented Jul 22, 2024

Uh oh!

kou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Watson1978 commented Jul 23, 2024

Uh oh!

kou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

otegami left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Watson1978 commented Jul 23, 2024

Uh oh!

otegami left a comment

Choose a reason for hiding this comment