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 failing test cases for autodiscovery #265

Draft
wants to merge 4 commits into
base: trunk
Choose a base branch
from

Conversation

jkmassel
Copy link
Contributor

@jkmassel jkmassel commented Aug 15, 2024

Adds some test cases for issues I've seen on real sites:

  • Sometimes caching is so aggressive that there's no Link header – we could probably fix that by speculatively loading /wp-json and seeing if it works. Worst-case, we'd need an entire HTML parser to read the site and see if it's there.
  • IDN URLs can be a bit weird, we should make sure that's covered (it seems to work fine!)
  • Sites will use compression in their responses, and the test runner needs to handle it (so I've included all of reqwest's options related to that)

@jkmassel jkmassel changed the title Add failing test cases for auto discovery Add failing test cases for autodiscovery Aug 15, 2024
@jkmassel jkmassel force-pushed the add/autodiscovery-failing-tests branch from 8a0a72a to afc1054 Compare August 15, 2024 21:20
Comment on lines +169 to +181
pub trait AssertError {
type Item;
fn assert_error(self) -> Self::Item;
}

impl<T: std::fmt::Debug, E: std::error::Error> AssertError for Result<T, E> {
type Item = E;

fn assert_error(self) -> E {
assert!(self.is_err(), "Request was successful");
self.unwrap_err()
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is probably not going to be common enough to have a trait extension. Also, the Request was successful might throw us of in the test logs, because it's not clear that it was expected to fail.

In most cases, we need to validate that the request failed with a certain error for which we already have AssertWpError trait. So, at least for now, I think we should individually implement this assertion and pass in a message that'll point out the issue. If there is enough common usage, then we can implement a specialized one like AssertWpError.

I realize that we discussed me taking over these PRs and implement them as I see fit, but I just wanted to drop my thoughts here, so if I end up changing it, it doesn't come out of nowhere. 😅

@oguzkocer
Copy link
Contributor

Sometimes caching is so aggressive that there's no Link header – we could probably fix that by speculatively loading /wp-json and seeing if it works. Worst-case, we'd need an entire HTML parser to read the site and see if it's there.

We should probably disable caching for these requests with CACHE_CONTROL & CDN_CACHE_CONTROL headers. What do you think @jkmassel?

@jkmassel
Copy link
Contributor Author

Sometimes caching is so aggressive that there's no Link header – we could probably fix that by speculatively loading /wp-json and seeing if it works. Worst-case, we'd need an entire HTML parser to read the site and see if it's there.

We should probably disable caching for these requests with CACHE_CONTROL & CDN_CACHE_CONTROL headers. What do you think @jkmassel?

This doesn't work :(

I can't get anything like curl "https://aggressive-caching.wpmt.co/" -H 'Cache-Control: private, no-store, max-age=0' to give the Link header – I suspect we'll need an HTML parser

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