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

Refactor some logic in "CDP_Validator" class #586

Merged
merged 1 commit into from
May 17, 2022
Merged

Conversation

ono-max
Copy link
Member

@ono-max ono-max commented Mar 26, 2022

Improve error messages in ValidationError

Use the hash map to get ruby classes from JSON type instead of the method

test/support/protocol_utils.rb Outdated Show resolved Hide resolved
cs = [cs] unless cs.is_a? Array
unless cs.any?{|c| property.is_a? c}
raise ValidationError, "Expected property `#{name}` to be kind of #{cs.join}, but it was #{property.class}"
end
Copy link
Member

Choose a reason for hiding this comment

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

Can we extract this part into a method? It'll be useful for validating test helper's result too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, I'm not sure what you try to do the method.
Could you explain more in detail about it?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry "validate" isn't a correct word here. I mean in assert_locals_result, I needed a way to convert the value type from response into the value type Ruby can understand (https://github.com/ruby/debug/pull/559/files#r831671288). And I feel there could be a helper method with this change too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I understand it.

Copy link
Member Author

@ono-max ono-max Mar 26, 2022

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

It's already Ruby objects when validate method is called.
here: https://github.com/ruby/debug/blob/master/test/support/protocol_utils.rb#L494

Copy link
Member Author

Choose a reason for hiding this comment

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

Does that answer your question?

Copy link
Member

Choose a reason for hiding this comment

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

I asked about this because I want to see if there's a way to simplify the type conversion logic. Specifically, I want to to check returns boolean as "true" or true.

And based on the types I recorded in all CDP tests, we never validate any boolean types:

property: "1:4:/private/var/folders/yg/hnbymwxd5pn7v94_clc59y6r0000gn/T/debug\\-20220328\\-77422\\-5eaac\\.rb|file:///private/var/folders/yg/hnbymwxd5pn7v94_clc59y6r0000gn/T/debug\\-20220328\\-77422\\-5eaac\\.rb" (String), types: [String]
property: [{:scriptId=>"1", :lineNumber=>4}] (Array), types: [Array]
property: "1:7:/private/var/folders/yg/hnbymwxd5pn7v94_clc59y6r0000gn/T/debug\\-20220328\\-77422\\-5eaac\\.rb|file:///private/var/folders/yg/hnbymwxd5pn7v94_clc59y6r0000gn/T/debug\\-20220328\\-77422\\-5eaac\\.rb" (String), types: [String]
property: [{:scriptId=>"1", :lineNumber=>7}] (Array), types: [Array]
property: "1:4:/private/var/folders/yg/hnbymwxd5pn7v94_clc59y6r0000gn/T/debug\\-20220328\\-77422\\-sxkdan\\.rb|file:///private/var/folders/yg/hnbymwxd5pn7v94_clc59y6r0000gn/T/debug\\-20220328\\-77422\\-sxkdan\\.rb" (String), types: [String]
property: [{:scriptId=>"1", :lineNumber=>4}] (Array), types: [Array]
property: {:type=>"number", :description=>"2", :value=>2, :objectId=>"0.22894592780960088"} (Hash), types: [Hash]
property: "number" (String), types: [String]
property: 2 (Integer), types: [Object]
property: "2" (String), types: [String]
property: "0.22894592780960088" (String), types: [String]
property: {:type=>"number", :description=>"4", :value=>4, :objectId=>"0.06395617303308232"} (Hash), types: [Hash]
property: "number" (String), types: [String]
property: 4 (Integer), types: [Object]
property: "4" (String), types: [String]
property: "0.06395617303308232" (String), types: [String]
property: {:type=>"number", :description=>"3", :value=>3, :objectId=>"0.35474411590333166"} (Hash), types: [Hash]
property: "number" (String), types: [String]
property: 3 (Integer), types: [Object]
property: "3" (String), types: [String]
property: "0.35474411590333166" (String), types: [String]
property: "1:2:/private/var/folders/yg/hnbymwxd5pn7v94_clc59y6r0000gn/T/debug\\-20220328\\-77422\\-fkadvt\\.rb|file:///private/var/folders/yg/hnbymwxd5pn7v94_clc59y6r0000gn/T/debug\\-20220328\\-77422\\-fkadvt\\.rb" (String), types: [String]
property: [{:scriptId=>"1", :lineNumber=>2}] (Array), types: [Array]
property: "1:3:/private/var/folders/yg/hnbymwxd5pn7v94_clc59y6r0000gn/T/debug\\-20220328\\-77422\\-k9e6b2\\.rb|file:///private/var/folders/yg/hnbymwxd5pn7v94_clc59y6r0000gn/T/debug\\-20220328\\-77422\\-k9e6b2\\.rb" (String), types: [String]
property: [{:scriptId=>"1", :lineNumber=>3}] (Array), types: [Array]
property: {:type=>"number", :description=>"2", :value=>2, :objectId=>"0.7218502316156181"} (Hash), types: [Hash]
property: "number" (String), types: [String]
property: 2 (Integer), types: [Object]
property: "2" (String), types: [String]
property: "0.7218502316156181" (String), types: [String]
property: {:type=>"number", :description=>"3", :value=>3, :objectId=>"0.9270118849216029"} (Hash), types: [Hash]
property: "number" (String), types: [String]
property: 3 (Integer), types: [Object]
property: "3" (String), types: [String]
property: "0.9270118849216029" (String), types: [String]
property: {:type=>"number", :description=>"1", :value=>1, :objectId=>"0.7065503792269908"} (Hash), types: [Hash]
property: "number" (String), types: [String]
property: 1 (Integer), types: [Object]
property: "1" (String), types: [String]
property: "0.7065503792269908" (String), types: [String]
property: {:type=>"object", :description=>"#<Method: Object(Kernel)#p(*)>", :objectId=>"0.8446957153608285", :className=>"Method"} (Hash), types: [Hash]
property: "object" (String), types: [String]
property: "Method" (String), types: [String]
property: "#<Method: Object(Kernel)#p(*)>" (String), types: [String]
property: "0.8446957153608285" (String), types: [String]
property: "1:30:/private/var/folders/yg/hnbymwxd5pn7v94_clc59y6r0000gn/T/debug\\-20220328\\-77422\\-xc906m\\.rb|file:///private/var/folders/yg/hnbymwxd5pn7v94_clc59y6r0000gn/T/debug\\-20220328\\-77422\\-xc906m\\.rb" (String), types: [String]
property: [{:scriptId=>"1", :lineNumber=>30}] (Array), types: [Array]
property: {:type=>"object", :description=>"Abc", :objectId=>"0.9745359026680118", :className=>"Module"} (Hash), types: [Hash]
property: "object" (String), types: [String]
property: "Module" (String), types: [String]
property: "Abc" (String), types: [String]
property: "0.9745359026680118" (String), types: [String]
property: {:type=>"object", :description=>"Abc::Def123", :objectId=>"0.047485239832737336", :className=>"Class"} (Hash), types: [Hash]
property: "object" (String), types: [String]
property: "Class" (String), types: [String]
property: "Abc::Def123" (String), types: [String]
property: "0.047485239832737336" (String), types: [String]
property: {:type=>"object", :description=>"Abc::Def123::Ghi", :objectId=>"0.8535166024221634", :className=>"Class"} (Hash), types: [Hash]
property: "object" (String), types: [String]
property: "Class" (String), types: [String]
property: "Abc::Def123::Ghi" (String), types: [String]
property: "0.8535166024221634" (String), types: [String]
property: {:type=>"object", :description=>"Abc::Def123::Ghi", :objectId=>"0.40720994916108144", :className=>"Class"} (Hash), types: [Hash]
property: "object" (String), types: [String]
property: "Class" (String), types: [String]
property: "Abc::Def123::Ghi" (String), types: [String]
property: "0.40720994916108144" (String), types: [String]
property: {:type=>"object", :description=>"Abc", :objectId=>"0.8723083291879542", :className=>"Module"} (Hash), types: [Hash]
property: "object" (String), types: [String]
property: "Module" (String), types: [String]
property: "Abc" (String), types: [String]
property: "0.8723083291879542" (String), types: [String]
property: {:type=>"object", :description=>"Abc::Def123", :objectId=>"0.37859743077279573", :className=>"Class"} (Hash), types: [Hash]
property: "object" (String), types: [String]
property: "Class" (String), types: [String]
property: "Abc::Def123" (String), types: [String]
property: "0.37859743077279573" (String), types: [String]
property: {:type=>"object", :description=>"Abc::Def123", :objectId=>"0.8528331746615713", :className=>"Class"} (Hash), types: [Hash]
property: "object" (String), types: [String]
property: "Class" (String), types: [String]
property: "Abc::Def123" (String), types: [String]
property: "0.8528331746615713" (String), types: [String]
property: {:type=>"number", :description=>"2", :value=>2, :objectId=>"0.6968169068815614"} (Hash), types: [Hash]
property: "number" (String), types: [String]
property: 2 (Integer), types: [Object]
property: "2" (String), types: [String]
property: "0.6968169068815614" (String), types: [String]
property: {:type=>"number", :description=>"3", :value=>3, :objectId=>"0.34527121101942637"} (Hash), types: [Hash]
property: "number" (String), types: [String]
property: 3 (Integer), types: [Object]
property: "3" (String), types: [String]
property: "0.34527121101942637" (String), types: [String]
property: {:type=>"number", :description=>"4", :value=>4, :objectId=>"0.8129917432476875"} (Hash), types: [Hash]
property: "number" (String), types: [String]
property: 4 (Integer), types: [Object]
property: "4" (String), types: [String]
property: "0.8129917432476875" (String), types: [String]

Patch used to record types:

      def validate_ res, props
        return if props.nil?

        props.each{|prop|
          name = prop[:name].to_sym
          if property = res[name]
            types = get_ruby_type prop[:type]
            + File.open("./types.txt", "a") do |f|
            +  f.puts("property: #{property.inspect} (#{property.class.name}), types: #{types}")
            + end
            types.each{|type|
              raise ValidationError, "Expected #{property} to be kind of #{types.join}" unless property.is_a? type
            }
            validate_ property, prop[:properties]
          else
            raise ValidationError, "Expected to include `#{name}` in responses" unless prop.fetch(:optional, false)
          end
        }
      end

So here are my questions based on the result:

  1. There are no boolean types sent back. Is it because our tests don't have enough coverage? Or we shouldn't expect boolean anyway and so we actually don't need to validate it?
  2. Integers seem to be sent with "any" type, so they're always validated with Object instead of Integer. Is this expected?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it because our tests don't have enough coverage?

Yes.

Copy link
Member Author

@ono-max ono-max Apr 2, 2022

Choose a reason for hiding this comment

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

Integers seem to be sent with "any" type, so they're always validated with Object instead of Integer. Is this expected?

In :value, yes. It's based on the JSON file in CDP. Maybe you misunderstand it because lineNumber is not validated? Although lineNumber should be validated as Integer, it's not currently because there is a bug in the Array case. I'll fix it.

Improve error messages in ValidationError

Use the hash map to get ruby classes from JSON type instead of the method
@ko1 ko1 added this to the v1.6.0 milestone Mar 30, 2022
@ko1 ko1 merged commit c6c22fb into ruby:master May 17, 2022
@ono-max ono-max deleted the patch-62 branch October 24, 2022 11:15
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