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

Basic verification for responses #13

Merged
merged 21 commits into from
Jul 13, 2023
Merged

Basic verification for responses #13

merged 21 commits into from
Jul 13, 2023

Conversation

sztomi
Copy link
Owner

@sztomi sztomi commented Jul 1, 2023

This PR adds a new crate called integration_tests which has a build script that generates tests from all response variants. The tests have two forms:

For variants that don't hold data:

  #[test]
  fn validate_terminatethreads_response() {
    let resp = Response {
      request_seq: 1,
      success: true,
      message: None,
      body: Some(ResponseBody::TerminateThreads),
    };
    let value = resp_to_value(&resp);
    assert!(SCHEMA.is_valid(&value));
  }

For variants that hold data:

  #[test]
  fn validate_threads_response() {
    let resp = Response {
      request_seq: 1,
      success: true,
      message: None,
      body: Some(ResponseBody::Threads(ThreadsResponse::default())),
    };
    let value = resp_to_value(&resp);
    assert!(SCHEMA.is_valid(&value));
  }

So this relies on the Default impl for each type. Obviously, this will mean that any Options will be None so many fields are not actually tested. But this is a good starting point. syn can be used to discover the struct fields and set them to generate more kinds of tests.

DRAFT: currently I cannot make the tests fail by renaming a field completely so I'm doing something wrong.

@sztomi
Copy link
Owner Author

sztomi commented Jul 1, 2023

@pileghoff I can't add you as a reviewer apparently, but would you mind taking a look? (it might be that GH only allows adding maintainers which I would still be happy to add you as).

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.

Other than two small things, it looks good 👍🏽

integration_tests/Cargo.toml Outdated Show resolved Hide resolved
integration_tests/build.rs Outdated Show resolved Hide resolved
@pileghoff
Copy link
Collaborator

Also, feel free to add me as a maintainer 😀

@sztomi
Copy link
Owner Author

sztomi commented Jul 4, 2023

Finally, some progress (and lots of broken responses, unsurprisingly):

image

@sztomi sztomi force-pushed the verify-schema branch 3 times, most recently from a8ad05f to 2fe4a7e Compare July 6, 2023 21:52
@sztomi sztomi marked this pull request as ready for review July 6, 2023 21:52
@sztomi
Copy link
Owner Author

sztomi commented Jul 6, 2023

@pileghoff the generated tests are now passing, please give this another go if you don't mind. Most of the time the required fix was not serializing None/null. While I think that shouldn't necessarily break the schema validation, I'm fine with doing this because the generated JSON will be smaller.

I will likely cut a release after this is merged.

For a bit more sophisticated testing I will generate structs with random data (that's not null). I don't know if there's already some crate out there that does that but in the worst case I can write a macro for it.

For testing the requests deserialization, a similar approach can be followed, but by generating JSON first and deserializing that.

@pileghoff
Copy link
Collaborator

I hope to look at it over the weekend 👍🏽 I was actually about to ask about making a new release with all the new fixes. I think after this is merged is a great time.

Regarding generating test data, would it be worth creating an example application that could talk to some of the most popular editors, so we could record the interaction and get some data that way? Then we could ensure that we were not only compliant with the spec, but that we also (sort of) works with those editors, in case they have some quirks or additions?

@sztomi
Copy link
Owner Author

sztomi commented Jul 7, 2023

Regarding generating test data, would it be worth creating an example application that could talk to some of the most popular editors, so we could record the interaction and get some data that way?

Good idea and probably more useful than a synthetic test for the requests. They might do things slightly differently and dap-rs should work with all of them.

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.

The tests are passing, which is the most important thing 🥇 I have a few cosmetic suggestion, but its great to see how many errors you were able to catch here, so we dont have to fix them individually as users encounters them.

integration_tests/debugAdapterProtocol.json Outdated Show resolved Hide resolved
integration_tests/src/lib.rs Outdated Show resolved Hide resolved
integration_tests/build.rs Show resolved Hide resolved
integration_tests/src/lib.rs Outdated Show resolved Hide resolved
@sztomi sztomi requested a review from pileghoff July 10, 2023 20:02
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.

I think it looks good 👍🏽

@sztomi sztomi merged commit 4d1dd8b into main Jul 13, 2023
@sztomi sztomi deleted the verify-schema branch July 13, 2023 09:19
@sztomi sztomi mentioned this pull request Jul 14, 2023
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