test/unit/sstring_test: do not test standard library

[tchaikov]

according to related section of C++ standard draft at $string.ends.with, see https://eel.is/c++draft/string.ends.with, it is equivalent to

return basic_string_view(data() + (size() - rlen), rlen) == x;

where rlen is the smaller of size() and x.size().

so, if the parameter equal to the string, this function should return true.

in existing test of sstring, we are verifying the behavior of standard library, which is not implemented by us, neither do we change its behavior. so there is no need to test it.

see also fc86e8c, which introduced these tests.

[nyh]

This test was deliberate: to prove, in code, that the behavior we are testing for in this test is indeed the behavior of std:string. I think we should do more of these comparisons, not less.

[tchaikov]

This test was deliberate: to prove, in code, that the behavior we are testing for in this test is indeed the behavior of std:string. I think we should do more of these comparisons, not less.

yeah, i knew this was deliberate. but again, we should not test standard library for the reasons i explained in the commit message. if we go in this direction further, we would have to include part of libstdc++'s test suite or the one included by libc++ for testing basic_string even its behavior is defined by the standard spec, this does not make sense at all.

[nyh]

On the contrary, it makes a lot of sense. The best way to test sstring's compatibility with std::string is to run the same test on both. Otherwise, the test itself might be incorrect! Same as our ability to run CQL tests on both Scylla and Cassandra.
I agree it's not necessary to run the std::string version of the test every time, but what does it hurt really? It's not hard and slow as running Cassandra.

[tchaikov]

On the contrary, it makes a lot of sense.

only when it has relatively good coverage. or when we plan to continuously improve in this direction.

what does it hurt really?

it does not hurt. it's like writing hundreds of assert(1); in the code. it's like a compliance test suite for a library which only touches a tiny piece of it. if it is designed to perform the compatibility, it should be at least have good coverage, otherwise it does not help to build the confidence. and the funny thing is that we are testing the reference implementation (whatever the standard library Seastar is compiled with).

this single test alone does not help unless we invest more time to so that it has a good coverage of the related sections in the standard.

anyway, let's wait for more inputs.

[nyh]

I agree it only covers a small part. But it's better than nothing in my opinion- and if I remember correctly it's not the first test that also checks std::string. In my opinion we should have more of those, not less.

[nyh]

I looked, and saw we also have a test case in the same file, test_find_sstring_compatible whose purpose is to check sstring::find() is compatible with std::string::find() (sadly the test comment forgets to mention that it reproduces issue #1001).

So now we have a second test that reproduces a compatibility error against std::string.

If you ask me, we should have a lot more of these. In fact, in my opinion every single test in sstring_test.cc should have been run on both std::string and sstring to confirm the behavior is identical. The fact we only do this in two tests doesn't mean they need to be deleted - maybe it means more should be added?

[tchaikov]

if we wanted to go in this direction, i think we should parameterize the tests, and allow user to run with sstring or with std::string, but apparently, we are repeating them in this very case.

anyway, since i failed to get more inputs from other maintainers, i am closing this change.