Users integration tests & error handling

[oguzkocer]

This PR implements integration tests for the /users endpoint and adds error handling as part of it. It contains some changes that are not directly related to tests, but they address issues found during testing. It's been a bit difficult to juggle different branches with the squash merge strategy, so they all ended up being in this single PR.

The tests are split into 3 parts; immutable tests, mutable tests and errors.

Mutable Tests

These tests will result in some kind of change in the server which means they have to be run sequentially and then the server needs to be restored to its original state before the next test can be run. If we were to run multiple of these tests at the same time, the change from one of them might influence another. Furthermore, trying to restore the server multiple times can result in mysql errors.

There are several components to this setup:

1. We have a dump-mysql (the name can be improved) command that will use mysqldump to create a backup of the database. In the future, this command should be run as part of test-server command, but that's not handled in this PR.
2. There is a new wp_db module in wp_networking's integration tests. This module will handle all our DB interactions in a controlled way. Specifically, wp_db::run_and_restore method is provided that will give access to the WordPressDb. This struct implements the Drop trait and as part of it, calls the make restore-mysql command. Since we are not allowing direct access to WordPressDb, we get to control when the db is restored from a central place.
3. This setup doesn't force the tests to be run sequentially, as it doesn't seem possible in a straightforward way at the moment. There is an RFC for this with a nightly implementation, but for now, we have to manually use the --test-threads 1 option of cargo test to run our mutable tests.

Immutable Tests

These tests are, for the most part, simple GET requests to ensure that our requests are successful. We don't do any specific assertions for these as they wouldn't add too much value for the added complexity. We initially considered implementing only a few tests, and not covering all the different parameters, but it turned out that we had some errors in our implementation and these tests were valuable in identifying them. They are also very quick to implement and run. You can see some of the fixes in this PR which I'll not go into detail as they should be self explanatory.

Error Tests

These are arguably the most valuable of them all, because they not only validate that we are handling the errors correctly, but they also provide a great documentation for how a certain error might be received. In my experience, this was always a big pain point for feature development and a big reason why errors are not handled well. They take a significant time investment to implement, but that's because they are not documented at all, so the time investment is well worth it.

---

It's possible to discuss the implementation details here, but I think I am going to leave them to line comments as they come up in the PR review, so please feel free to raise any questions you might have.

---

To Test

• make test-server
• make dump-mysql
• cargo test --test '*' -- --nocapture --test-threads 1

Note that the CI integration is currently WIP.

Also note that the CI failure of Build xcframework step is due to the nightly toolchain version.

[crazytonyli]

Accessing database directly feels like extra complexity to me. Are there any major benefits in that, compared to using REST API like api().retrieve_user(created_user.id.0)?

[oguzkocer]

This is something we have discussed extensively with @jkmassel - even prior to us starting the wordpress-rs project. The main issue with that approach is that testing one part of the implementation depends on another. In this case the create & retrieve would be the dependants, which may not seem much, but when the whole testing suite is designed that way we had a lot of issues with it in FluxC project. So, we decided to keep our integration tests to a single request per test.

We could consider removing the DB assertions all together and rely on whether the request was successful or not - similar to how immutable tests work. However, querying the DB has a very little overhead and I think it might help us catch some minor mistakes - especially during the initial implementation. It also gives me extra confidence when I refactor things, so I'd prefer to keep the DB assertions.

The main overhead of these tests is restoring the DB, but that part will need to be done regardless of the assertions - as also mentioned in the PR description.

[crazytonyli]

What'd be the use case for this function? WPApiError.RestError has a status_code that is grabbed from the HTTP response, which seems to be a duplication of this function value which is hard-coded in the library.

I also wonder if this library should assume certain HTTP status code is always returned for certain error code string {"code": "..."}. Even though they are one to one map in current and past WordPress versions, that doesn't feel like part of REST API contract and may chance in future versions.

For example, returning HTTP 500 when failing to create an user (Self::UserCreate => 500) doesn't feel right to me. Someone may "correct" the error response to return 403 (like Self::CannotCreateUser => 403).

[oguzkocer]

The main goal of having this here is documentation. I do use them for assertions in the integration tests, but I don't expect them to be directly used by the clients. That's why it's not currently exported to UniFFI. We could move it to the testing suite, but I like being able to see this information immediately as I add new errors.

---

I am not sure I understand the second part of your comment. Are you suggesting that we should modify the status code of errors as we see fit? If so, I really don't like that idea. Our job is to package and convey information between server and client, not to modify it. We make some minor changes along the way, but those are all about packaging - such as using an enum or removing a parameter because it always needs to be a certain value. (such as force in deleting users) I don't think we should ever alter information.

However, I think you might be asking that whether having this information might be a problem if it changes in the future WordPress versions - or maybe some error type might be reported with multiple status codes. I've given some thought to that while I implemented it and I have some ideas to get around the issues - but ultimately, because this is only supposed to be used internally, it shouldn't cause any major issues.

The information is currently public to Rust clients - which is a bit of an issue, but I think we can get around that - maybe by using cargo features? 🤔

[crazytonyli]

The main goal of having this here is documentation.

Can you elaborate in what way it's used as documentation? Does that mean it should never be called internally?

Do you think writing it as an actual documentation would satisify your needs? Like

+   /// ..... HTTP status code: 400.
    #[serde(rename = "rest_cannot_create_user")]
    CannotCreateUser,

I think you might be asking that whether having this information might be a problem if it changes in the future WordPress versions

Yes. This is what I meant.

The information is currently public to Rust clients.

I'm more concerned about it's used incorrectly internally. Presuming my understanding of documentation is correct, where this function should not be called at runtime. Code like if apiErrorCode.status_code == 400 { ... } looks legit, but may produce incorrect result if they should be checking the real HTTP response status code instead. There is no way to tell this code is incorrect, unless PR reviewers know status_code is meant for documentation and should not be called.

[oguzkocer]

I'll move it to integration tests. Would that address your concerns?

[crazytonyli]

👍 Yep, thanks for that!

[oguzkocer]

Addressed in 277d7b1.

[crazytonyli]

Is it possible to merge these two structs into one? Like maybe adding an WPRestErrorCode.Unknown { code: String } case?

[oguzkocer]

I've thought the same thing but I couldn't get it to work without using a custom deserializer and I didn't want to invest too much time into it. If we all agree that it's how we should handle the fallback, I can look into it again.

[crazytonyli]

I think that'd be the most nature API design.

I'm not familiar with serde, so I don't know how complicated it'd be to implement such API. However, I think it can be implemented in a super boring way: do not derive Deserialise for WPRestErrorCode, and use a function to turn string to WPRestErrorCode, which would be the same amount of code to write #[serde(rename = "...")].

[oguzkocer]

I kind of like the rename stuff because the definition is closer 🤔

We discussed this a bit with @jkmassel on the call today and we thought it would be OK to leave it like this for a version or two, so we get to see other types of errors before we settle on the final design. Would that work for you as well?

[crazytonyli]

Sounds good to me!