[PATCH 3/4] ws2_32/tests: Add comprehensive socket option validity tests

[Zebediah Figura (she/her)]

On 8/11/21 1:02 PM, Alex Henrie wrote:
> On Wed, Aug 11, 2021 at 9:18 AM Zebediah Figura (she/her)
> <zfigura at codeweavers.com> wrote:
>>
>> This is a good idea in principle, but a few things bother me about it:
>>
>> (1) the tables are hard to read (could maybe be solved with indentation)?
> 
> I'd be happy to indent the tables, but honestly they would be a lot
> cleaner if Wine always called SetLastError(WSAENOPROTOOPT) in
> setsockopt when an option is not recognized. (It already sets
> WSAENOPROTOOPT for SOL_SOCKET options, just not for network-level
> options.) Can I just send a patch to do that, or do the error code
> tests need to be added first?

I think either approach is reasonable; it'd be nice if it can be added 
to the series so that I can see what the end result looks like up front.

>> (2) it would probably make sense to split get/set into separate tables,
>> and not to split tcp/udp? I dunno, looks potentially simpler.
>>
>> (3) It's a bit ambiguous in many cases whether an operation is supported
>> or not. I.e. it'd be nice to have *successful* calls to most of these.
>> At least WSAENOPROTOOPT is relatively unambiguous, but I can't tell if a
>> given operation always returns WSAEINVAL (like, say, the hardcoded IPv4
>> option 41) or only returns WSAEINVAL because we're not using the right
>> arguments.
>>
>> Which is to say that maybe a table isn't that great after all :-/
> 
> In these tests, WSAEINVAL generally means that an option is supported
> on TCP but not UDP or vice-versa.
> 
> Since the setsockopt tests depend on getting a valid result from
> getsockopt, how about if we just skip calling setsockopt if we expect
> getsockopt to fail? That would be a little less comprehensive, but the
> tables would be a lot shorter.

That seems reasonable to me, but this does break testing set-only options.

Actually now that I see how you're calling setsockopt(), I guess EINVAL 
is less unconvincing; we probably expect setsockopt() to work with the 
same data...

Hmm, do we need TODO_OPT? Can't we derive that from the error code?

> 
>> (4) It's also kind of unclear what /* win7 */ means: broken on win7, or
>> before win7, or up to and including win7, or what?
> 
> Like in most other Wine tests, the comment tells the most recent
> version of Windows where the option doesn't work correctly. For
> example, /* win7 */ means that the option does not work on Windows 7,
> but it works on Windows 8 and later.
> 
> By the way, the IPV6_RECVTCLASS patch has its own set of tests. Would
> you approve it if I resend it by itself while we're still working on
> the more comprehensive tabular tests?

Probably, yes.

> 
> Thanks for the feedback,
> 
> -Alex
>