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

Improve assert_repl_result for CDP #951

Merged
merged 1 commit into from
Apr 21, 2023

Conversation

st0012
Copy link
Member

@st0012 st0012 commented Mar 29, 2023

The current implementation doesn't work when the result is a boolean type because the helper calls #inspect on the values of JAVASCRIPT_TYPE_TO_CLASS_MAPS, which is an array for boolean types.

Example failure:

<["[TrueClass, FalseClass]"]> was expected to include
<"FalseClass">.

Since JAVASCRIPT_TYPE_TO_CLASS_MAPS is never used in other places, we can just store the String representation of the class name in the hash and avoid calling #inspect on the values.

The current implementation doesn't work when the result is a boolean type
because the helper calls `#inspect` on the values of `JAVASCRIPT_TYPE_TO_CLASS_MAPS`,
which is an array for boolean types.

Example failure:

```
<["[TrueClass, FalseClass]"]> was expected to include
<"FalseClass">.
```

Since `JAVASCRIPT_TYPE_TO_CLASS_MAPS` is never used in other places, we
can just store the String representation of the class name in the hash
and thus avoid calling `#inspect` on the values.
Copy link
Member

@ono-max ono-max left a comment

Choose a reason for hiding this comment

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

Thank you 👍

@ono-max
Copy link
Member

ono-max commented Mar 30, 2023

By the way, I think that JAVASCRIPT_TYPE_TO_CLASS_MAPS is not the best name.
Do you think that JAVASCRIPT_TYPE_TO_RUBY_CLASS_MAPS is easier to understand?

@ko1 ko1 merged commit 8290437 into ruby:master Apr 21, 2023
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