Skip to content

Conversation

@tingwei628
Copy link
Contributor

As title, I added MSSQL driver.

Thanks!

@imiric
Copy link

imiric commented May 17, 2021

Hi @tingwei628, thanks for your contribution and terribly sorry for the extremely late response. 😞 I missed the GitHub notification and don't check this project often.

We'd definitely like to merge this. Would you mind fixing the merge conflicts and maybe adding a test.js for it? I'd like to improve the testing situation for this extension so that we could have integration tests for all supported RDBMSs, and run them in CI. But in the meantime moving the existing test.js for SQLite into a tests directory and adding the MSSQL variant would improve the situation. There is an official SQL Server Docker image, so it should be straightforward to test and eventually move to CI.

Thanks in advance!

@tingwei628
Copy link
Contributor Author

tingwei628 commented May 17, 2021

Hi @imiric, I've already fixed the conflict!
There're two questions here:

  1. How do i add test.js ?
  2. Should I delete vendor/ ?

Thanks!

@imiric
Copy link

imiric commented May 17, 2021

Thanks for the quick response!

  1. Similar to the existing test.js for SQLite, but using MSSQL syntax. And moving both files to a tests subdirectory. I'll add a Postgres one and maybe hook up some integration tests in CI.

  2. I removed it recently in f43fcf1.

@tingwei628
Copy link
Contributor Author

tingwei628 commented May 17, 2021

Hi @imiric, I've added tests/ in 1678c4a. Also I renamed original test.js into sqlite3_test.js and added mssql_test.js in tests/ directory.
Thanks !

Copy link

@imiric imiric left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I tested it with the SQL Server Docker image, and it works fine after I specified the connection string correctly.

So I'll merge this, and make some more changes, like upgrading go-mssqldb to the latest version, since 0.10.0 was released recently.

@@ -0,0 +1,24 @@
import sql from 'k6/x/sql';

const db = sql.open("sqlserver", "./test.db");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The second argument should be a connection string to showcase how it's specified for MS SQL, but it's OK, I'll fix it when I add the CI tests.

@imiric imiric merged commit 9511f02 into grafana:master May 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants