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

Expand tests of resource resolution #309

Merged
merged 6 commits into from
Apr 10, 2023

Conversation

bcsgh
Copy link
Contributor

@bcsgh bcsgh commented Apr 6, 2023

Add some more test for resource resolution.

This includes

Comment on lines 386 to 390
#ifdef CASE_INSENSITIVE
LT_CHECK_EQ(true, ws->register_resource("ok", &ok2));
#else
LT_CHECK_EQ(false, ws->register_resource("ok", &ok2));
#endif
Copy link
Contributor Author

Choose a reason for hiding this comment

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

After a bit more digging, I think these are actually backwards. And both in fact end up returning false.

tl;dr: http_endpoint::operator < is unconditionally case-insensitive:

bool http_endpoint::operator <(const http_endpoint& b) const {
COMPARATOR(url_normalized, b.url_normalized, std::toupper);
}

That's going to be a pain to fix as it's something users could be inadvertently depending on.

Also add a comment that the non `-DCASE_INSENSITIVE` testcases is probably wrong.
Really this should be assigned to an issue report, not a person, because nobody has agreed to fix it yet.
@bcsgh bcsgh marked this pull request as ready for review April 6, 2023 05:23
@etr etr merged commit 205aea0 into etr:master Apr 10, 2023
@bcsgh bcsgh deleted the bcsgh/expand-tests-resource-resolution branch April 11, 2023 02:03
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