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

Add fake response testing #14

Merged
merged 8 commits into from
Jul 29, 2023
Merged

Add fake response testing #14

merged 8 commits into from
Jul 29, 2023

Conversation

sztomi
Copy link
Owner

@sztomi sztomi commented Jul 14, 2023

This is a continuation of #13 using fake. This still doesn't test all possible response structures, but it tests a lot more. It might be useful to implement a form of fuzzing where the seeds can be recorded and similar tests can be ran. That shouldn't run in Github Actions though, because it will be potentially unstable.

@sztomi sztomi requested a review from pileghoff July 14, 2023 21:04
@@ -244,109 +258,151 @@ pub struct Capabilities {
pub supports_single_thread_execution_requests: Option<bool>,
}

#[derive(Serialize, Deserialize, Debug, Default, Clone)]
pub struct CustomValue(Value);
Copy link
Owner Author

Choose a reason for hiding this comment

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

This is added to be able to implement the Dummy trait for Value.

@sztomi sztomi marked this pull request as ready for review July 14, 2023 22:35
Copy link
Collaborator

@pileghoff pileghoff left a comment

Choose a reason for hiding this comment

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

Overall it looks good, but i must admit, i haven't looked too deep into how all this works. Two things i noticed while playing around:

  1. I wont detect if i remove Option<_> from a member (but it will detect if you add one, so thats good!). Not sure what can be done about that.
  2. Because it does not test the entire message, but only the body, it did not catch that the raw string variant of ResponseMessage is wrong, as we need flatten it. Currently its wrapped inError(_). Im not sure how important it is to test the entire message in general, especially with all the issues you are encountering with the schema.

@sztomi
Copy link
Owner Author

sztomi commented Jul 24, 2023

Because it does not test the entire message, but only the body, it did not catch that the raw string variant of ResponseMessage is wrong, as we need flatten it.

Can you give me an example?

@pileghoff
Copy link
Collaborator

Because it does not test the entire message, but only the body, it did not catch that the raw string variant of ResponseMessage is wrong, as we need flatten it.

Can you give me an example?

fn main() {
  let a = Response {
    request_seq: 1,
    success: false,
    message: Some(ResponseMessage::Error("test".to_string())),
    body: None,
  };
  println!("{}", serde_json::to_string(&a).unwrap());
}

prints

{"request_seq":1,"success":false,"message":{"Error":"test"}}

but should print

{"request_seq":1,"success":false,"message":"test"}

@pileghoff
Copy link
Collaborator

I also just now noticed that because we moved the dap source folder, cargo run --examples no longer works, as the examples folder also needs to be moved into the dap folder.

@sztomi
Copy link
Owner Author

sztomi commented Jul 26, 2023

Thanks, I filed an issue about the error flattening. It's a good point that we probably should test entire messages. That might be useful future work or we might ultimately end up just iterating enough (and adding individual regression tests) that we weed out all of these mistakes.

I'll look into making the examples work again. I remember this being painful in workspaces.

@sztomi sztomi merged commit cdace09 into main Jul 29, 2023
@sztomi sztomi deleted the fake-response-test branch July 29, 2023 14:15
@sztomi
Copy link
Owner Author

sztomi commented Jul 29, 2023

oh that somehow didn't get autosquashed when I rebased ... I'll amend on main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants