Fix tests (mostly ODBC) #676

mmcnabb-vms · 2025-10-06T23:47:41Z

Fixed a bunch of ODBC tests . Entire test suite now passes for me using SQL Server 2019 and 2022, in Linux and Windows (and VMS).

The test a_defncopy failed in Windows because it hardcoded invoking ..\tsql from the test binary location, but in Windows the binary directories are prefixed by the build type, so that path should be ..\..\Debug\tsql.

I fixed it by removing the path prefix, so it will grab a binary from the PATH. When we're running the test suite we should have staged what we just built onto PATH anyway (in Windows this lets it find all the needed DLLs; in Linux can be done by configure --prefix etc.)

… path instead Signed-off-by: Matt McNabb <[email protected]>

Signed-off-by: Matt McNabb <[email protected]>

…ot server date Signed-off-by: Matt McNabb <[email protected]>

Signed-off-by: Matt McNabb <[email protected]>

mmcnabb-vms · 2025-10-07T00:41:19Z

OK, the Linux build didn't like the change to defncopy test . As mentioned in the PR , the test didn't work in other operating systems because it hardcodes a path used by the Linux build system.

Reviewing the build log -- the build installs tsql to /tmp/freetds/bin/ , however it is still not found by defncopy test. @freddy77 would it be possible to update the build container to include /tmp/freetds/bin/ on its PATH ?

freddy77 · 2025-10-10T09:44:46Z

OK, the Linux build didn't like the change to defncopy test . As mentioned in the PR , the test didn't work in other operating systems because it hardcodes a path used by the Linux build system.

Reviewing the build log -- the build installs tsql to /tmp/freetds/bin/ , however it is still not found by defncopy test. @freddy77 would it be possible to update the build container to include /tmp/freetds/bin/ on its PATH ?

Which operating systems are we talking about here? Tests should not use the installed binary, but the one compiled in the source directory, so the PATH should not include final installation path.

mmcnabb-vms · 2025-10-12T20:28:58Z

OK, the Linux build didn't like the change to defncopy test . As mentioned in the PR , the test didn't work in other operating systems because it hardcodes a path used by the Linux build system.
Reviewing the build log -- the build installs tsql to /tmp/freetds/bin/ , however it is still not found by defncopy test. @freddy77 would it be possible to update the build container to include /tmp/freetds/bin/ on its PATH ?

Which operating systems are we talking about here? Tests should not use the installed binary, but the one compiled in the source directory, so the PATH should not include final installation path.

Talking about Windows build configured with cmake -G "Visual Studio 17 2022" (but running tests via ctest commandline). The defncopy test hardcodes ../tsql as the path to built executable, but for that environment the path should be ..\..\Debug\tsql or ..\..\Release\tsql depending on build type, etc.

I am hoping for a better solution than having to modify the C code of the test to build a path as defined by the build system, do you have any other ideas?

… staging path instead" This reverts commit c149624. The fix did not work in the GitHub test container for Linux

mmcnabb-vms · 2025-10-12T21:18:26Z

@freddy77 I've reverted my "fix" for defncopy test... perhaps we can accept this PR now and leave Windows tests for a github issue. It seems like the Windows appveyor doesn't actually run any tests anyway

mmcnabb-vms · 2025-10-14T23:38:13Z

@freddy77 I have just noticed a mistake in my commit "ODBC - fix Hebrew/CN in Windows", the code hexdump(stderr, "Got : ", out[p[n].num], len); should have strlen(out[p[n].num]) instead of len .

I haven't pushed a fix as I am guessing from your commit to appveyor you might be working on fixing Windows iconv instead of accepting this workaround ?

freddy77 · 2025-10-15T08:50:12Z

Yes, I fixed the iconv compilation to allow some other changes. Use of iconv is strongly recommended, that is not using it should be avoided.

mmcnabb-vms · 2025-10-16T03:58:07Z

Yes, I fixed the iconv compilation to allow some other changes. Use of iconv is strongly recommended, that is not using it should be avoided.

OK, I guess my change can still be accepted then? It will still test the iconv if they HAVE_ICONV but won't give error if they don't.

freddy77 · 2025-10-16T21:11:09Z

Yes, I fixed the iconv compilation to allow some other changes. Use of iconv is strongly recommended, that is not using it should be avoided.

OK, I guess my change can still be accepted then? It will still test the iconv if they HAVE_ICONV but won't give error if they don't.

yes

freddy77 · 2025-10-16T21:13:42Z

src/odbc/sqlwchar.c

+/* Compile-time check that sizes are defined correctly
+ * (otherwise the #if in odbc.h can malfunction)
+ */
+typedef char sizecheck_sqlwchar[(SIZEOF_SQLWCHAR == sizeof(SQLWCHAR)) ? 1 : -1];


Better to use TDS_COMPILE_CHECK macro, defined in freetds/macros.h.

freddy77 · 2025-10-16T21:14:23Z

include/freetds/odbc.h

 #if SIZEOF_SQLWCHAR != SIZEOF_WCHAR_T
 size_t sqlwcslen(const SQLWCHAR * s);
+#else
+#define sqlwcslen(s) wcslen(s)


There's no mention of this change in the commit message

I moved the definition of sqlwstr_buf and the SQLWSTR macros that use it, inside #ifdef ENABLE_ODBC_WIDE since they are not referenced outside of ENABLE_ODBC_WIDE ; but sqlwcslen needs to be outside that block as it is called without guards from sql2tds.c.

Re. the change in the same commit on sqlwchar.c -- IMHO it's more robust for sqlwchar.c to use #ifndef sqlwcslen, than repeat the tests from the header that led to sqlwcslen being defined (DRY principle)

freddy77 · 2025-10-16T21:15:15Z

src/odbc/unittests/utf8_2.c

 	{ 0, NULL, NULL },
 };

+static void hexdump(FILE* fp, char const* prefix, void const* p, size_t n)


static void hexdump(FILE* fp, const char* prefix, const void* p, size_t n)

freddy77 · 2025-10-16T21:16:02Z

src/odbc/unittests/utf8_2.c

+static void hexdump(FILE* fp, char const* prefix, void const* p, size_t n)
+{
+	fprintf(fp, "%s", prefix);
+	unsigned char const* cp = p;


Just use p, no reason to create a copy. Also, do not mix declarations and code.

in Standard C, pointer to void cannot be incremented or dereferenced - I prefer not to rely on compiler extensions if it's easily avoidable. But you could change the function signature , feel free to make cosmetic changes to my PR

Yes, I didn't see the implicit cast. Okay, just don't mix declarations and code, some compilers still complains.

freddy77 · 2025-10-16T21:18:43Z

src/odbc/unittests/utf8_2.c

 	odbc_command("SELECT hebrew, cn FROM #tmp ORDER BY i");
-
+#else
+	fprintf(stderr, "Bypassing Hebrew MBCS test since iconv not installed (Forcing server to send UTF-8).\n");


The server will send UTF-16, not UTF-8, it's the client that converts to UTF-8, as built-in iconv supports both.

freddy77 · 2025-10-16T21:21:17Z

src/odbc/unittests/c2string.c

 	case SQL_C_SBIGINT:
 		assert(in_len == sizeof(SQLBIGINT));
-		sprintf(s, "%" PRId64, (int64_t) IN.bi);
+		sprintf(s, "%" PRId64, *(int64_t *)&IN.bi);


That expression is odd... the fact that int64_t and PRId64 exist means that a 64 bit type exists... but the pointer dereference seems to indicate IN.bi is not a number but possibly a structure. So it looks like some setting is wrong in the configuration.

This arose because of a mistake in UnixODBC sqltypes.h for my system that led to SQLBIGINT being defined as a struct containing two 32-bit ints , despite the fact that the compiler does support 64-bit ints. (I have since fixed sqltypes.h to use the 64-bit int on my system, that will be a separate PR for UnixODBC!).

But you are correct in your analysis that if the compiler does not support int64_t then my fix would not work , so let's just drop this change from the PR .

If you used Cmake probably this was needed ce7fad6 (already in master branch)

freddy77 · 2025-10-16T21:22:10Z

src/ctlib/unittests/common.c


-	return (*common_pwd.user && *common_pwd.server && *common_pwd.database)
+	/* (MM) user is not necessary for MSSQL trusted logins, for example */
+	return (/* *common_pwd.user && */ *common_pwd.server && *common_pwd.database)


Why left it commented out? Are you sure of the change?

Well - if using MSSQL Trusted Connection then it is correct for user to be the empty string; the server authenticates based on your Windows user login identity. So in order to run the test suite against a MSSQL server that only uses Trusted Connection and not username/password login, the test should not require a username to be present . (I did run the test suite in this mode on my Windows system).

I'm presuming this code was in place to try and provide better diagnostics for when the user inadvertently forgot to configure PWD , however perhaps just checking for server and database still suits that need.

freddy77 · 2025-10-16T21:22:57Z

src/ctlib/unittests/common.c

 	read_login_info();

-	return (*common_pwd.user && *common_pwd.server && *common_pwd.database)
+	/* (MM) user is not necessary for MSSQL trusted logins, for example */


I would just say

/* user is not necessary for MSSQL trusted logins */

freddy77 · 2025-10-16T21:23:34Z

src/odbc/unittests/tokens.c

 	}
 	printf("Fake server bound at port %d\n", port);

+	setup_override();		/* Forces odbc_read_login_info() to find login details for our fake server instead */


that's odd... why it works for me?

Not sure... I am testing with TDSPWDFILE defined in my environment to point to the PWD for my test, are you doing the same? (as opposed to finding the hardcoded default path to PWD).

With the original order of these functions I get the output:

query : use TempDB Line 1: unknown command 'use'

whereas with my suggested order, I get the output

query : use tempdb Testing SQLFetch, line 425

and it proceeds to work correctly. Debugging in the fake thread proc shows that the original order receives as its first command use TempDB while the swapped order receives rowfmt, then row, then info etc. I am not sure where use TempDB is coming from.

In the original order it seems to me that the "override" details are never used -- because the first call to odbc_read_login_info sets common_pwd->initialized = true ; so even though setup_override is subsequently called which redefines TDSPWDFILE, the odbc_connect() call never reads the new TDSPWDFILE because of common_pwd->initialized already being set.

If I use the original order but insert common_pwd.initialized = common_pwd.tried_env = false; just before calling odbc_connect(), then the test works .

freddy77 · 2025-10-16T21:24:43Z

src/odbc/unittests/connect.c

+	/* Connect using SQLConnect() with Server, User and Password from PWD file
+	 * NOTE: In Windows this looks up DSN in HKCU\Software\ODBC\ODBC.INI, with fallback to HKLM
+     */
+		printf("SQLConnect(server=%s, ...) connect..\n", common_pwd.server);


weird indentation

I probably don't have whitespace/tab settings configured properly in my editor to match your conventions for this project, please adjust to suit

don't worry, it's only a single instance

freddy77 · 2025-10-16T21:25:45Z

src/odbc/unittests/connect.c

 	sprintf(tmp, "DSN=%s;UID=%s;PWD=%s;DATABASE=%s;", common_pwd.server,
 		common_pwd.user, common_pwd.password, common_pwd.database);
-	CHKDriverConnect(NULL, T(tmp), SQL_NTS, (SQLTCHAR *) tmp, sizeof(tmp)/sizeof(SQLTCHAR), &len, SQL_DRIVER_NOPROMPT, "SI");
+	/* note: final argument "SI" means CHKDriverConnect will exit() if the result is not Success or Sucess With Info */


these strings are used everywhere, I would say it's base knowledge, the comment looks redundant to me

freddy77 · 2025-10-16T21:26:22Z

src/odbc/unittests/connect.c

 		common_pwd.user, common_pwd.password, common_pwd.database);
-	CHKDriverConnect(NULL, T(tmp), SQL_NTS, (SQLTCHAR *) tmp, sizeof(tmp)/sizeof(SQLTCHAR), &len, SQL_DRIVER_NOPROMPT, "SI");
+	/* note: final argument "SI" means CHKDriverConnect will exit() if the result is not Success or Sucess With Info */
+	rc = CHKDriverConnect(NULL, T(tmp), SQL_NTS, (SQLTCHAR *) tmp, sizeof(tmp)/sizeof(SQLTCHAR), &len, SQL_DRIVER_NOPROMPT, "SI");


are you using rc value?

Looks like I'm not, must have inadvertently left that in after debugging why it was failing on my system.

freddy77 · 2025-10-16T21:28:07Z

src/odbc/unittests/connect.c

-#ifdef _WIN32
-	if (get_entry("SERVER")) {
-		init_connect();
-		sprintf(tmp,
-			"DRIVER=FreeTDS;SERVER=%s;UID=%s;PWD=%s;DATABASE=%s;",
-			entry, common_pwd.user, common_pwd.password, common_pwd.database);
-		if (get_entry("TDS_Version"))
-			sprintf(strchr(tmp, 0), "TDS_Version=%s;", entry);
-		if (get_entry("Port"))
-			sprintf(strchr(tmp, 0), "Port=%s;", entry);
-		rc = CHKDriverConnect(NULL, T(tmp), SQL_NTS, (SQLTCHAR *) tmp, sizeof(tmp) / sizeof(SQLTCHAR), &len,
-				      SQL_DRIVER_NOPROMPT, "SIE");
-		if (rc == SQL_ERROR) {
-			printf("Unable to open data source (ret=%d)\n", rc);
-		} else {
-			++succeeded;
-		}
-		odbc_disconnect();
-	}
-#endif


why did you remove this?

Well, this test invokes SQLDriverConnect with DRIVER=FreeTDS , but typically that means it'll find the driver already installed on your system, unless you specifically modified the odbcinst.ini registry key to have an entry [FreeTDS] that points to the development driver we just built. Some of the tests have code for Linux to generate a temporary odbcinst.ini but Windows ODBC uses a registry key called Odbcinst.ini instead, it does not use environment variable ODBCINSTINI.

I decided to remove because: (a) as just described, this test is not testing what we just built; and (b) even if it did, the test is redundant with regard to the pass condition succeeded < 3 as there are also 3 other tests which should succeed. (Although maybe this is not actually the case for Sybase, I have only tested with MSSQL on this round).

freddy77 · 2025-10-16T21:32:20Z

src/odbc/unittests/connect.c

+#ifdef _WIN32
+		printf("Try from admin command prompt:\n\todbcconf /A {INSTALLDRIVER \"FreeTDS | Driver = C:\\Program Files(x86)\\FreeTDS\\bin\\tdsodbc.dll\"}"
+			"\n(replace path with your installation path for tdsodbc.dll if necessary)\n");
+#endif


Maybe for Windows we could use functions to add the driver? Like SQLInstallDriverEx ? Maybe using a different name like FreeTDS_test.

FreeTDS_Test sounds like a good option to me -- then we would get helpful error messages instead of silently using the wrong driver. The same concept could perhaps be used in all of the "front door" tests (i.e. the ones that go in via SQLDriverConnect).

mmcnabb-vms · 2025-10-17T00:37:40Z

@freddy77 let me know how you'd like to proceed from here -- e.g. will you commit your own versions of my commits once you're happy (as you did for Issue 511 & the ASE bulk copy) , or if you want me to do anything else on this branch such as implement your suggestions?

freddy77 · 2025-10-17T13:06:37Z

@freddy77 let me know how you'd like to proceed from here -- e.g. will you commit your own versions of my commits once you're happy (as you did for Issue 511 & the ASE bulk copy) , or if you want me to do anything else on this branch such as implement your suggestions?

I'm pretty busy at the moment, so probably you will be faster than me.

mmcnabb-vms · 2025-10-20T23:32:29Z

@freddy77 let me know how you'd like to proceed from here -- e.g. will you commit your own versions of my commits once you're happy (as you did for Issue 511 & the ASE bulk copy) , or if you want me to do anything else on this branch such as implement your suggestions?

I'm pretty busy at the moment, so probably you will be faster than me.

@freddy77 OK - and would you prefer new commits, or force-rewrite this branch with suggestions implemented ?

freddy77 · 2025-10-23T20:39:33Z

@freddy77 OK - and would you prefer new commits, or force-rewrite this branch with suggestions implemented ?

I saw you easily added new commits. I can easily merge fixes before merging. Possibly asking you if I did a nice job. Not requiring to write fixup commits.

mmcnabb-vms added 10 commits October 6, 2025 13:01

Tests: a_defncopy had hardcoded Linux path for tsql.exe - use staging…

c149624

… path instead Signed-off-by: Matt McNabb <[email protected]>

Tests: username not required, e.g. if using MSSQL Trusted Connection

222503f

Signed-off-by: Matt McNabb <[email protected]>

Tests: ODBC - tokens.c setup override needs to be called first

6966943

Signed-off-by: Matt McNabb <[email protected]>

Tests: ODBC - fix Connect test

b0b542f

Signed-off-by: Matt McNabb <[email protected]>

Tests: ODBC - In test output for 'type' clarify that Errors are expected

3af8ab1

Signed-off-by: Matt McNabb <[email protected]>

ODBC: compile-time check for SIZEOF_SQLWCHAR etc. being correct

b362159

Signed-off-by: Matt McNabb <[email protected]>

Tests: ODBC - convert C_TIME to DATETIME should insert client date, n…

8712725

…ot server date Signed-off-by: Matt McNabb <[email protected]>

Tests: ODBC - Correct data type for length parameter

7896d60

Signed-off-by: Matt McNabb <[email protected]>

Tests: ODBC - Handle systems without 'long long'

62c516c

Signed-off-by: Matt McNabb <[email protected]>

Tests: ODBC - fix Hebrew/CN test in Windows

efceeb8

Signed-off-by: Matt McNabb <[email protected]>

Revert "Tests: a_defncopy had hardcoded Linux path for tsql.exe - use…

e1cc311

… staging path instead" This reverts commit c149624. The fix did not work in the GitHub test container for Linux

freddy77 reviewed Oct 16, 2025

View reviewed changes

Fix tests (mostly ODBC) #676

Are you sure you want to change the base?

Fix tests (mostly ODBC) #676

Uh oh!

Conversation

mmcnabb-vms commented Oct 6, 2025

Uh oh!

mmcnabb-vms commented Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

freddy77 commented Oct 10, 2025

Uh oh!

mmcnabb-vms commented Oct 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mmcnabb-vms commented Oct 12, 2025

Uh oh!

mmcnabb-vms commented Oct 14, 2025

Uh oh!

freddy77 commented Oct 15, 2025

Uh oh!

mmcnabb-vms commented Oct 16, 2025

Uh oh!

freddy77 commented Oct 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mmcnabb-vms Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

freddy77 Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mmcnabb-vms commented Oct 7, 2025 •

edited

Loading

mmcnabb-vms commented Oct 12, 2025 •

edited

Loading

mmcnabb-vms Oct 17, 2025 •

edited

Loading

freddy77 Oct 16, 2025 •

edited

Loading

mmcnabb-vms commented Oct 20, 2025 •

edited

Loading