-
Notifications
You must be signed in to change notification settings - Fork 138
Verify CAP/DAP requests automatically #599
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
Conversation
I like this idea 👍 |
Could you be more specific? |
If you verify the
Even if the fix itself may be simple, fixing this will also affect all the recorded raw tests. So I think it's better to do it in another PR. |
264f8c5
to
3146782
Compare
|
||
def assert_disconnect_result | ||
def cleanup_reader | ||
@reader_thread.raise Detach |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice refactoring.
send_request 'Runtime.terminateExecution' | ||
res = find_crt_cdp_response | ||
assert_cdp_response 'Runtime.terminateExecution', res | ||
send_cdp_request 'Runtime.terminateExecution' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, I forgot to add cleanup_reader
method. Could you add it here, too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated 👍
16f36ea
to
6f6d41e
Compare
6f6d41e
to
ba7a5fd
Compare
@ono-max can you give this another look? thx |
Great! LGTM |
Since all protocol responses should be verified except for a few exceptions (e.g. DAP's
stepBack
), the verification should be performed automatically. This can prevent situations like #559 (comment) and also make the API easier to use (getting response from the same method call).There are 2 exceptions though. Both of them are DAP requests:
stepBack
doesn't return response "yet".stackTrace
's response doesn't match the schema. I think this is something to be fixed in another PR.This refactor can actually centralize those exceptions and make them easier to manage. And sooner or later they should go anyway.