Re: [PATCH 1/3] msvcp110: Add tr2_sys__Symlink implementation and test.(try 2)

[Piotr Caban]

On 02/18/16 10:54, YongHao Hu wrote:
> - delete_flag = 1;
> - for(i=0; i<sizeof(tests)/sizeof(tests[0]); i++) {
> -
>    errno = 0xdeadbeef;
> -
>    SetLastError(0xdeadbeef);
> -
>    ret = p_tr2_sys__Symlink(tests[i].existing_path, tests[i].new_path);
> -
>    if(ret==ERROR_PRIVILEGE_NOT_HELD || ret==ERROR_INVALID_FUNCTION) {
> -
>        delete_flag = 0;
> -
>        win_skip("Privilege not held or symbolic link not supported, skipping symbolic link tests.\n");
> -
>        break;

If second symlink creation fails we'll end up with some not deleted 
files while all tests are succeeding. I'm not sure if it's a real issue. 
I also think that it will be better if you remove the delete_flag 
variable. Something like this will be better in my opinion:

ret = p_tr2_sys__Symlink(tests[i].existing_path, tests[i].new_path);
if(!i && (ret==ERROR_PRIVILEGE_NOT_HELD || ret==ERROR_INVALID_FUNCTION)) {
     win_skip(...);
     ret = p_tr2_sys__Remove_dir("tr2_test_dir");
     ok(ret == 1, "tr2_sys__Remove_dir(): expect 1 got %d\n", ret);
     return;
}
ok(ret == ERROR_SUCCESS, ...);
> -
>    if(tests[i].is_todo)
> -
>        todo_wine ok(ret == tests[i].last_error, "tr2_sys__Symlink(): test %d expect: %d, got %d\n", i+1, tests[i].last_error, ret);
> -
>    else
> -
>        ok(ret == tests[i].last_error, "tr2_sys__Symlink(): test %d expect: %d, got %d\n", i+1, tests[i].last_error, ret);

Please use todo_wine_if here.

Thanks,
Piotr

[PATCH 1/3] msvcp110: Add tr2_sys__Symlink implementation and test.(try 2)

[YongHao Hu]

Hi,

On 16/2/18 下午10:10, Piotr Caban wrote:
> On 02/18/16 10:54, YongHao Hu wrote:
> > - delete_flag = 1;
> > - for(i=0; i<sizeof(tests)/sizeof(tests[0]); i++) {
> > -
> >    errno = 0xdeadbeef;
> > -
> >    SetLastError(0xdeadbeef);
> > -
> >    ret = p_tr2_sys__Symlink(tests[i].existing_path, 
> >
> > tests[i].new_path);
> > -
> >    if(ret==ERROR_PRIVILEGE_NOT_HELD || 
> >
> > ret==ERROR_INVALID_FUNCTION) {
> > -
> >        delete_flag = 0;
> > -
> >        win_skip("Privilege not held or symbolic link not 
> >
> > supported, skipping symbolic link tests.\n");
> > -
> >        break;
>
> If second symlink creation fails we'll end up with some not deleted 
> files while all tests are succeeding. I'm not sure if it's a real 
> issue. I also think that it will be better if you remove the 
> delete_flag variable. Something like this will be better in my opinion:
>
> ret = p_tr2_sys__Symlink(tests[i].existing_path, tests[i].new_path);
> if(!i && (ret==ERROR_PRIVILEGE_NOT_HELD || 
> ret==ERROR_INVALID_FUNCTION)) {
>     win_skip(...);
>     ret = p_tr2_sys__Remove_dir("tr2_test_dir");
>     ok(ret == 1, "tr2_sys__Remove_dir(): expect 1 got %d\n", ret);
>     return;
> }
> ok(ret == ERROR_SUCCESS, ...);

This approach is better.
> > -
> >    if(tests[i].is_todo)
> > -
> >        todo_wine ok(ret == tests[i].last_error, 
> >
> > "tr2_sys__Symlink(): test %d expect: %d, got %d\n", i+1, 
> > tests[i].last_error, ret);
> > -
> >    else
> > -
> >        ok(ret == tests[i].last_error, "tr2_sys__Symlink(): test 
> >
> > %d expect: %d, got %d\n", i+1, tests[i].last_error, ret);
>
> Please use todo_wine_if here.

I don't notice that todo_wine_if was implemented on Feb 9 2016.
Do I need to simplify some tests before by using todo_wine_if?
Thank you.

[PATCH 1/3] msvcp110: Add tr2_sys__Symlink implementation and test.(try 2)

[Piotr Caban]

Hi,

On 02/18/16 16:12, YongHao Hu wrote:
> Do I need to simplify some tests before by using todo_wine_if?

You can send this patches first if you prefer.

Thanks,
Piotr