[PATCH 1/2] d3dx10/tests: Add tests for D3DX10GetImageInfoFromResource{A, W}().

[Ziqing Hui]

On 11/12/20 5:47 PM, Matteo Bruni wrote:
> On Thu, Nov 12, 2020 at 9:10 AM Nikolay Sivov <nsivov at codeweavers.com> wrote:
>> On 11/12/20 5:05 AM, Ziqing Hui wrote:
>>> On 11/12/20 3:00 AM, Matteo Bruni wrote:
>>>> On Mon, Nov 9, 2020 at 5:13 AM Ziqing Hui <zhui at codeweavers.com> wrote:
>>>>> Signed-off-by: Ziqing Hui <zhui at codeweavers.com>
>>>>> ---
>>>>>  dlls/d3dx10_43/tests/d3dx10.c | 78 +++++++++++++++++++++++++++++++++++
>>>>>  1 file changed, 78 insertions(+)
>>>> Both patches look generally good. I have a couple of small comments /
>>>> questions though.
>>>>
>>>> +    ret = CopyFileW(current_module_path, resource_module_path, FALSE);
>>>> +    if (!ret)
>>>> +    {
>>>> +        if (winetest_debug > 1)
>>>> +            trace("CopyFileW failed, error %u.\n", GetLastError());
>>>> +        return NULL;
>>>> +    }
>>>>
>>>> Maybe a skip() in place of the debug-level-dependent trace() is more
>>>> appropriate?
>>> I thought skip() should be used when system lacks something. It should not be used when error happens. Here, CopyFile failed is a error, we expect this CopyFile to be succeeded in all situations. What do you think?
> I guess at the moment skip isn't quite right either, somehow I thought
> it just skipped the test if the resource creation fails but clearly
> that's not the case. I would still change the test. I don't think it's
> reasonably possible for the file copy to fail so I'd just drop the
> whole if (!ret) check, or alternatively use a ok().
>
OK, I'll take this approach, which is using a ok() like this:

    ret = CopyFileW(current_module_path, resource_module_path, FALSE);
    ok(ret, "CopyFileW failed, error %u.\n", GetLastError());
    if (!ret)
        return NULL;

>>>> +    SetFileAttributesW(resource_module_path, FILE_ATTRIBUTE_ARCHIVE);
>>>>
>>>> Is this one necessary? If so, in what case?
>>> This line is necessary, because If we don't set the file attribute, the file would be read only. Using FILE_ATTRIBUTE_NORMAL here is also OK. Do you think it's better to use FILE_ATTRIBUTE_NORMAL?
> Why though? I'd expect the file you just created with CopyFileW() to
> be writable. At least that seems to be the case for me on Win 10.
>
On my win10 machine, the file created by CopyFile is read only. Because I found that the following BeginUpdateResouce, DeleteFile will fail without this line.

>> Is it possible to simply embed test images to the test binary directly,
>> and remove all temporary files logic?
> It certainly is, although I like the structure of the test and how it
> avoids duplicating stuff.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20201113/3a449371/attachment.htm>