search

XAMPPRocky / octocrab

A modern, extensible GitHub API Client for Rust.
Other
1.09k stars 264 forks source link

Test models #32

Open kellda opened 4 years ago

kellda commented 4 years ago

I'd like to add tests against samples in the GitHub Docs reference for structs in the ´models´ module.

These samples are often hunderds of lines long, so I'm not sure where to put these tests.

XAMPPRocky commented 4 years ago

Thank you for your issue! How do you intend on including getting the samples? I don't think including them inline would be the right thing to do. The problem I encounter with the models is when there's a field that's unspecified as optional (e.g. because the object was created before that field existed), and having unit tests comparing those samples doesn't really help test that.

I'm open to adding more testing for the models, but I would want to be something that's maintainable, and will help future proof it rather than testing its current correctness. I think testing the samples once and opening a PR with any required changes to get them to work would be enough.

kellda commented 4 years ago

I was thinking to put the samples either inline or in a separate file (and use something like include_str!). Fetching them while testing seems quite difficult and unreliable, if that's what you meant.

I don't really understand what you mean by

when there's a field that's unspecified as optional (e.g. because the object was created before that field existed)

Could you give me an example ?

As I understand it, the GitHub API is made to be backward compatible and so fields are only added, never removed. That means that if it is currenly correct, it will stay so in the future (by default, serde_json just ignore fields present in the JSON data but not in the Rust type).

kenr commented 3 years ago

Just some input regarding models for communicating with the GitHub REST API.

Would it be interesting to use the OpenAPI definitions for the GitHub REST API? I see that they have OpenAPI specifications in https://github.com/github/rest-api-description.

In theory the models could be automatically generated by various OpenAPI/Swagger crates that has this feature. I have not tested those crates in Rust, but I have done this previously in TypeScript/JS projects and it works well.

XAMPPRocky commented 3 years ago

In theory the models could be automatically generated by various OpenAPI/Swagger crates that has this feature. I have not tested those crates in Rust, but I have done this previously in TypeScript/JS projects and it works well.

Thank you for your comment. In the long term, yes I would like to move this crate to using OpenAPI for code generation, however I have a few caveats to that which is why I haven't pursued it further. Firstly, I would still want complete control over the code generation process, I need the codegen to support the Rust features and module structure I need, and I don't want to depend on some other library to generate all of that for me.

Secondly and relatedly, I'm also planning some improvements that will require a good bit of code churn in the Semantic API and in the request builders, and I'd like to get those changes merged to know they work before switching those to being auto generated.

That being said, if you are interested, I would be open to a contribution now that moves the models module to use OpenAPI for code generation, provided it fulfilled the first requirement I mentioned. If anyone is interested in taking this on, I think the first step would be to look at the various OpenAPI crates and post a short comparison here of any that could be suitable to use. Then once we've agreed on the library, the implementation can start.