On Thu, Nov 12, 2020 at 9:10 AM Nikolay Sivov nsivov@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@codeweavers.com wrote:
Signed-off-by: Ziqing Hui zhui@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().
- 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.
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.