Signed-off-by: Ziqing Hui zhui@codeweavers.com --- dlls/d3dx10_43/tests/d3dx10.c | 78 +++++++++++++++++++++++++++++++++++ 1 file changed, 78 insertions(+)
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?
+ SetFileAttributesW(resource_module_path, FILE_ATTRIBUTE_ARCHIVE);
Is this one necessary? If so, in what case?
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?
- 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?
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?
- 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?
Is it possible to simply embed test images to the test binary directly, and remove all temporary files logic?
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.
On 11/12/20 5:47 PM, Matteo Bruni wrote:
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().
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.
-
Matteo Bruni -
Nikolay Sivov -
Ziqing Hui