Re: [PATCH 2/2] kernel32/tests: Test Get/SetUserGeoID, GetUserDefaultGeoName, SetUserGeoName.

[Nikolay Sivov]

On 10/3/19 7:40 AM, João Diogo Ferreira wrote:
> This adds unit tests for these four functions:
>    GetUserGeoID();
>    SetUserGeoID();
>    GetUserDefaultGeoName();
>    SetUserGeoName().
>
> These are all functions I intend to improve and/or implement
> in the short future.
> Signed-off-by: João Diogo Craveiro Ferreira
>
> dlls/kernel32/tests/locale.c | 319 +++++++++++++++++++++++++++++++++++
>   1 file changed, 319 insertions(+)
>
> diff --git a/dlls/kernel32/tests/locale.c b/dlls/kernel32/tests/locale.c
> index 81e74531ea..e3e46bfb53 100644
> --- a/dlls/kernel32/tests/locale.c
> +++ b/dlls/kernel32/tests/locale.c
> @@ -108,6 +108,8 @@ static LANGID (WINAPI *pSetThreadUILanguage)(LANGID);
>   static LANGID (WINAPI *pGetThreadUILanguage)(VOID);
>   static INT (WINAPI *pNormalizeString)(NORM_FORM, LPCWSTR, INT, LPWSTR, INT);
>   static INT (WINAPI *pFindStringOrdinal)(DWORD, LPCWSTR lpStringSource, INT, LPCWSTR, INT, BOOL);
> +static int (WINAPI *pGetUserDefaultGeoName)(LPWSTR, int);
> +static BOOL (WINAPI *pSetUserGeoName)(PWSTR);
>
> static void InitFunctionPointers(void)
>   {
> @@ -145,6 +147,8 @@ static void InitFunctionPointers(void)
>     X(GetThreadUILanguage);
>     X(NormalizeString);
>     X(FindStringOrdinal);
> -
>
> X(GetUserDefaultGeoName);
> -
>
> X(SetUserGeoName);
>
> mod = GetModuleHandleA("ntdll");
>  X(RtlUpcaseUnicodeChar);
>
> @@ -4745,6 +4749,317 @@ static void test_CompareStringOrdinal(void)
>       }
>   }
>
> +static void test_GetUserGeoID(void)
> +{
> - GEOID id;
> - todo_wine {
> -
>    if (pGetUserDefaultGeoName && pSetUserGeoName)
> -
>    {
> -
>        ok(GEOID_NOT_AVAILABLE != GetUserGeoID(GEOCLASS_NATION),
> -
>            "GEOCLASS_NATION: should never return GEOID_NOT_AVAILABLE when GeoName API is available.\n");
> -
>        ok(GEOID_NOT_AVAILABLE != GetUserGeoID(GEOCLASS_REGION),
> -
>            "GEOCLASS_REGION: should never return GEOID_NOT_AVAILABLE when GeoName API is available.\n");
> -
>    }
> -
>    else
> -
>        win_skip("This platform allows GEOID_NOT_AVAILABLE to be returned.\n");
> - }
> -
> - id = GetUserGeoID(GEOCLASS_ALL);
> - ok(id == GEOID_NOT_AVAILABLE,
> -
>    "GEOCLASS_ALL: Expected GEOID_NOT_AVAILABLE, got %d.\n", id);
> - id = GetUserGeoID(12345);
> - ok(id == GEOID_NOT_AVAILABLE,
> -
>    "Gibberish argument: Expected GEOID_NOT_AVAILABLE, got %d.\n", id);
>
> +}
> -
> +static void test_GetUserDefaultGeoName(void)
> +{
> - todo_wine {
> -
>    if (pGetUserDefaultGeoName && pSetUserGeoName)
> -
>    {
> -
>        WCHAR *name = malloc(sizeof(WCHAR) * 10);
> -
>        int count = 0;
> -
> -
>        if (!name)
> -
>        {
> -
>            trace("Couldn't allocate 20 bytes to store geo name. Aborting test!\n");
> -
>            return;
> -
>        }
> -
> -
>        SetLastError(ERROR_SUCCESS);
> -
>        count = pGetUserDefaultGeoName(NULL, 10);
> -
>        ok(count == 0 && (GetLastError() == ERROR_INVALID_PARAMETER),
> -
>            "Expected count == 0, got %d; and expected last error = ERROR_INVALID_PARAMETER (87), got %d\n",
> -
>            count, GetLastError());
> -
> -
>        count = pGetUserDefaultGeoName(NULL, -1);
> -
>        ok(count == 0 && (GetLastError() == ERROR_INVALID_PARAMETER),
> -
>            "Expected count == 0, got %d; and expected last error = ERROR_INVALID_PARAMETER (87), got %d\n",
> -
>            count, GetLastError());
> -
> -
>        SetLastError(ERROR_SUCCESS);
> -
>        count = pGetUserDefaultGeoName(name, 0);
> -
>        ok(count > 0, "Expected character count > 0, got %d; last error was %d.\n", count, GetLastError());
> -
> -
>        count = pGetUserDefaultGeoName(name, 1);
> -
>        ok(count == 0 && (GetLastError() == ERROR_INSUFFICIENT_BUFFER),
> -
>            "Expected count == 0, got %d; and expected last error = ERROR_INSUFFICIENT_BUFFER (122), got %d\n",
> -
>            count, GetLastError());
> -
> -
>        count = pGetUserDefaultGeoName(name, -1);
> -
>        ok(count == 0 && (GetLastError() == ERROR_INVALID_PARAMETER),
> -
>            "Expected count == 0, got %d; and expected last error = ERROR_INVALID_PARAMETER (87), got %d\n",
> -
>            count, GetLastError());
> -
> -
>        free(name);
> -
> -
>        SetLastError(ERROR_SUCCESS);
> -
>        count = pGetUserDefaultGeoName(NULL, 0);
> -
>        ok(count > 0, "Expected character count > 0, got %d; last error was %d.\n", count, GetLastError());
> -
>        if (count)
> -
>        {
> -
>            name = malloc(count * sizeof(WCHAR));
> -
>            if (name)
> -
>            {
> -
>                int result = pGetUserDefaultGeoName(name, count);
> -
>                ok(result,
> -
>                "Expected character count == %d, got %d; last error was %d.\n",
> -
>                count, result, GetLastError());
> -
>                free(name);
> -
>            }
> -
>            else
> -
>                trace("Couldn't allocate %d bytes to store geo name.\n", count);
> -
>        }
> -
>    }
> -
>    else win_skip("GetUserDefaultGeoName: procedure not implemented on this platform.\n");
> - }
>
> +}

Please check function pointer first and return if it's unavailable, 
instead of creating large outer nesting block.

Regarding string buffers, for this test it should be enough to use fixed 
size local buffer, but if you must allocate, please use kernel32 heap 
functions.
> -
>
> +static void test_SetUserGeoID(void)
> +{
> - BOOL status;
> - const int geoname_supported = (pGetUserDefaultGeoName && pSetUserGeoName);
> -
> - const GEOID original_nation = GetUserGeoID(GEOCLASS_NATION);
> - const GEOID original_region = GetUserGeoID(GEOCLASS_REGION);
> - int original_name_count = 0; /* We only need the char count. */
> -
> - struct geopair {
> -
>    GEOID id;
> -
>    WCHAR name[4];
> - };
> -
> - struct testsuit {
> -
>    const char *const stage;
> -
>    struct geopair geo, geo_backup;
> -
>    GEOCLASS geoclass;
> - };
> -
> - struct testsuit const tests[] = {
> -
>    {"GEOCLASS_NATION", {193, {'P','T',0}}, {56, {'C','U',0}}, GEOCLASS_NATION},
> -
>    {"GEOCLASS_REGION", {21206, {'0','5','7',0}}, {10039880, {'0','2','9',0}}, GEOCLASS_REGION}
> - };
> -
> - struct testsuit const bad_args[] = {
> -
>    {"invalid GEOID -1", {GEOID_NOT_AVAILABLE}},
> -
>    {"invalid GEOID 1", {1}}
> - };
> -
> - if (geoname_supported)
> -
>    original_name_count = pGetUserDefaultGeoName(NULL, 0);
> -
> - /* Invalid args */
> - for (int i = 0; i < ARRAY_SIZE(bad_args); i++)
> - {
> -
>    BOOL status;
> -
> -
>    SetLastError(ERROR_SUCCESS);
> -
> -
>    status = SetUserGeoID(bad_args[i].geo.id);
> -
>    ok(!status &&
> -
>        (GetLastError() == ERROR_INVALID_PARAMETER || /* Vista+ */
> -
>         broken(GetLastError() == ERROR_SUCCESS)), /* WinXP */
> -
>    "Setting %s: got ret=%d (expected 0), error=%d (expected 87=ERROR_INVALID_PARAMETER)\n",
> -
>    bad_args[i].stage, status, GetLastError());
> - }
> -
> - for (int i = 0; i < ARRAY_SIZE(tests); i++)
> - {
> -
>    const struct geopair *geo;
> -
> -
>    if (original_nation != tests[i].geo.id)
> -
>        geo = &tests[i].geo;
> -
>    else
> -
>        geo = &tests[i].geo_backup;
> -
> -
>    SetLastError(ERROR_SUCCESS);
> -
>    ok(status = SetUserGeoID(geo->id),
> -
>        "%s: Setting to %d failed with last error: %d\n", tests[i].stage, geo->id, GetLastError());
> -
>    if (status)
> -
>    {
> -
>        const GEOID id = GetUserGeoID(tests[i].geoclass);
> -
>        const int status = (id == geo->id);
> -
>        ok(status, "%s: we just set it to %d but retrieved %d instead \n", tests[i].stage, geo->id, id);
> -
> -
>        if (status)
> -
>        {
> -
>            todo_wine {
> -
>                if (geoname_supported)
> -
>                {
> -
>                    const int count = pGetUserDefaultGeoName(NULL, 0);
> -
>                    WCHAR *const name = malloc(count * sizeof(WCHAR));
> -
> -
>                    if (name)
> -
>                    {
> -
>                        const int written = pGetUserDefaultGeoName(name, count);
> -
>                        const int status = (count == written);
> -
> -
>                        ok(status,
> -
>                            "%s: Failed to get geoname after setting ID to %d: got %d characters, expected %d\n",
> -
>                            tests[i].stage, id, written, count);
> -
>                        ok(!winetest_strcmpW(name, geo->name),
> -
>                            "%s: wrong geoname for ID %d: got %s, expected %s\n",
> -
>                            tests[i].stage, geo->id, wine_dbgstr_w(name), wine_dbgstr_w(geo->name));
> -
>                        free(name);
> -
>                    }
> -
>                    else if (count > 0)
> -
>                        trace("Couldn't allocate memory to store geoname string! (Size = %lu bytes)\n",
> -
>                            count * sizeof(WCHAR));
> -
>                }
> -
>                else
> -
>                    win_skip("%s: Can't check geoname: unsupported platform.\n", tests[i].stage);
> -
>            }
> -
>        }
> -
>    }
> - }
> -
> - /* Best attempt at restoring original values */
> - if (geoname_supported)
> - {
> -
>    if (original_name_count <= 3)
> -
>    {
> -
>        SetUserGeoID(original_region);
> -
>        SetUserGeoID(original_nation);
> -
>    }
> -
>    else
> -
>    {
> -
>        SetUserGeoID(original_nation);
> -
>        SetUserGeoID(original_region);
> -
>    }
> - }
> - else
> - {
> -
>    const GEOID neutral = 39070; /* World */
> -
>    original_region != GEOID_NOT_AVAILABLE ? SetUserGeoID(original_region) : SetUserGeoID(neutral);
> -
>    original_nation != GEOID_NOT_AVAILABLE ? SetUserGeoID(original_nation) : SetUserGeoID(neutral);
> - }

My understanding is that setting geoid is not something regular 
application would do.
> +}
> -
>
> +static void test_SetUserGeoName(void)
> +{
> - todo_wine
> - {
> -
>    if (pGetUserDefaultGeoName && pSetUserGeoName)
> -
>    {
> -
>        struct geopair {
> -
>                WCHAR *name;
> -
>                GEOID id;
> -
>            };
> -
> -
>        struct testsuit {
> -
>            const char *stage;
> -
>            BOOL exp_return;
> -
>            int exp_error;
> -
>            struct geopair geo, geo_backup;
> -
>            GEOCLASS geoclass;
> -
>        };
> -
> -
>        /* Known valid test nations and regions */
> -
>        struct geopair vietnam = {(WCHAR[]){'V','N',0}, 251};
> -
>        struct geopair brasil = {(WCHAR[]){'B','R',0}, 32};
> -
>        struct geopair west_europe = {(WCHAR[]){'1','5','5',0}, 10210824};
> -
>        struct geopair east_asia = {(WCHAR[]){'0','3','0',0}, 47600};
> -
> -
>        /* Invalid arguments */
> -
>        WCHAR too_long[] = {'w','e',' ','t','o','o',' ','l','o','n','g',0}; /* Names must have less than 3 chars */
> -
>        WCHAR too_short[] = {'P',0}; /* Names must be at least two characters */
> -
>        WCHAR wrong_un_m49[] = {'A','A','A',0}; /* UN M49 codes use numbers only */
> -
>        WCHAR wrong_iso_3166a2[] = {'1','2',0}; /* ISO 3166-1 alpha-2 country codes use the latin alphabet only */
> -
> -
>        const struct testsuit good_args[] = {
> -
>            {"nation geoname", TRUE, ERROR_SUCCESS, vietnam, brasil, GEOCLASS_NATION},
> -
>            {"region geoname", TRUE, ERROR_SUCCESS, west_europe, east_asia, GEOCLASS_REGION}
> -
>        };
> -
> -
>        const struct testsuit bad_args[] = {
> -
>            {"too long geoname", FALSE, ERROR_INVALID_PARAMETER, {too_long}},
> -
>            {"too short geoname", FALSE, ERROR_INVALID_PARAMETER, {too_short}},
> -
>            {"invalid UN M49 code", FALSE, ERROR_INVALID_PARAMETER, {wrong_un_m49}},
> -
>            {"invalid ISO 3166 code", FALSE, ERROR_INVALID_PARAMETER, {wrong_iso_3166a2}}
> -
>        };
> -
> -
>        const int original_count = pGetUserDefaultGeoName(NULL, 0);
> -
>        WCHAR *const original_name = malloc(original_count * sizeof(WCHAR));
> -
>        const GEOID original_nation = GetUserGeoID(GEOCLASS_NATION);
> -
>        const GEOID original_region = GetUserGeoID(GEOCLASS_REGION);
> -
> -
>        /* Get current values before testing */
> -
>        if (original_name)
> -
>        {
> -
>            if (original_count != pGetUserDefaultGeoName(original_name, original_count))
> -
>                trace("Couldn't get original geo name; restoring is not possible.\n");
> -
>        }
> -
>        else if (original_count)
> -
>            trace("Couldn't allocate %lu bytes to store original name.\n", sizeof(WCHAR) * original_count);
> -
> -
> -
>        for (int i = 0; i < ARRAY_SIZE(bad_args); i++)
> -
>        {
> -
>            BOOL status;
> -
>            SetLastError(ERROR_SUCCESS);
> -
> -
>            status = pSetUserGeoName(bad_args[i].geo.name);
> -
>            ok((status == bad_args[i].exp_return) && (GetLastError() == bad_args[i].exp_error),
> -
>              "Setting %s: Got ret=%d (expected %d) and lasterror=%d (expected %d).\n",
> -
>              bad_args[i].stage, status, bad_args[i].exp_return, GetLastError(), bad_args[i].exp_error);
> -
>        };
> -
> -
> -
>        for (int i = 0; i < ARRAY_SIZE(good_args); i++)
> -
>        {
> -
>            BOOL status;
> -
>            const struct geopair *geo;
> -
> -
>            if (winetest_strcmpW(original_name, good_args[i].geo.name))
> -
>                geo = &good_args[i].geo;
> -
>            else
> -
>                geo = &good_args[i].geo_backup;
> -
> -
>            SetLastError(ERROR_SUCCESS);
> -
> -
>            status = pSetUserGeoName(geo->name);
> -
>            ok((status == good_args[i].exp_return) && (GetLastError() == good_args[i].exp_error),
> -
>               "Setting geoname to %s: failed with ret=%d (expected %d) and lasterror=%d (expected %d).\n",
> -
>               wine_dbgstr_w(geo->name), status, good_args[i].exp_return, GetLastError(), good_args[i].exp_error);
> -
> -
>            if (status == good_args[i].exp_return)
> -
>            {
> -
>                const GEOID geoid = GetUserGeoID(good_args[i].geoclass);
> -
> -
>                ok(geoid == geo->id, "%s (geoclass %d): Wrong GeoID for geoname %s: got %d (expected %d).\n",
> -
>                    good_args[i].stage, good_args[i].geoclass, wine_dbgstr_w(geo->name), geoid, geo->id);
> -
>            }
> -
>        }
> -
> -
>        /* Best attempt at restoring original values */
> -
>        SetUserGeoID(original_region);
> -
>        SetUserGeoID(original_nation);
> -
>        pSetUserGeoName(original_name);
> -
>    }
> -
>    else win_skip("SetUserGeoName: procedure not implemented on this platform.\n");
> - }
>
> +}
> - static void test_GetGeoInfo(void)
> {
>     char buffA[20];
>
> @@ -6058,6 +6373,10 @@ START_TEST(locale)
>     test_SetThreadUILanguage();
>     test_NormalizeString();
>     test_SpecialCasing();
> - test_GetUserGeoID();
> - test_GetUserDefaultGeoName();
> - test_SetUserGeoID();
> - test_SetUserGeoName();
>  /* this requires collation table patch to make it MS compatible */
>  if (0) test_sorting();
> }