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.

Sign up for GitHub

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

Refactor some logic in "CDP_Validator" class #586

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 13 additions & 21 deletions test/support/protocol_utils.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -553,6 +553,15 @@ class CDP_Validator
class ValidationError < Exception
end

JSON_TYPE_TO_CLASS_MAPS = {
'string' => String,
'integer' => Integer,
'boolean' => [TrueClass, FalseClass],
'object' => Hash,
'array' => Array,
'any' => Object
}

def validate res, domain, command
@seen = []
pattern = get_cmd_pattern(domain, command)
Expand All @@ -567,34 +576,17 @@ def validate_ res, props
props.each{|prop|
name = prop[:name].to_sym
if property = res[name]
types = get_ruby_type prop[:type]
types.each{|type|
raise ValidationError, "Expected #{property} to be kind of #{types.join}" unless property.is_a? type
}
cs = Array JSON_TYPE_TO_CLASS_MAPS[prop[:type]]
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 •
edited
Loading

Choose a reason for hiding this comment

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

We can use https://github.com/ruby/debug/pull/587/files#diff-7beb1edf264a098b7aea10570c70da47e70af61e673a7455cd5090e3e83250e4R370 to convert :type to Ruby class.

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 •
edited
Loading

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.

validate_ property, prop[:properties]
else
raise ValidationError, "Expected to include `#{name}` in responses" unless prop.fetch(:optional, false)
end
}
end

def get_ruby_type type
case type
when 'string'
[String]
when 'integer'
[Integer]
when 'boolean'
[TrueClass, FalseClass]
when 'object'
[Hash]
when 'array'
[Array]
when 'any'
[Object]
end
end

def get_cmd_pattern domain, command
cmd = nil
# FIXME: Commonalize this part.
Expand Down