Stabilized db create for tests by ellbosch · Pull Request #441 · dbcli/mssql-cli

ellbosch · 2020-03-24T20:56:15Z

Closes #280.

This PR introduces retry logic to check the status of a db creation using sys.dm_operation_status.

pensivebrian

Couple of changes related to retry logic.

pensivebrian

Approved with a suggestion to move the master check to the calling function

ellbosch · 2020-04-04T00:35:20Z

@pensivebrian I've made the suggested changes, please let me know your thoughts.

We still need to fix the known issue in the Special module but I can submit that in a separate PR.

pensivebrian

We need to be able to run against on-premise databases as well as azure.

pensivebrian

We should fail the test with a clear error that database creation failed.

pensivebrian · 2020-04-07T18:58:26Z

    shutdown(client)

    # cleanup db just in case, then raise exception
    clean_up_test_db(test_db_name)


clean_up_test_db [](start = 4, length = 16)

This will most likely fail, so let's wrap in a try catch

pensivebrian · 2020-04-07T18:59:16Z


    # cleanup db just in case, then raise exception
    clean_up_test_db(test_db_name)
    raise AssertionError("DB creation failed.")


Can we put the failing exception in the assert message - this way we don't have look at logs

pensivebrian · 2020-04-07T19:03:44Z

+            if is_create_error or create_db_status == 'FAILED':
+                # log warning to console and cleanup db
+                warnings.warn('Test DB create failed with error: {0}'.format(status))
+                clean_up_test_db(test_db_name)


clean_up_test_db [](start = 16, length = 16)

I don't think we should we should try an cleanup here. Let's let the caller perform the cleanup.

pensivebrian · 2020-04-07T19:07:28Z

+        shutdown(client)
+        return test_db_name
+
    shutdown(client)


Can we put one shutdown call in a finally block?

pensivebrian

Why are we refactoring the globalization tests?

pensivebrian · 2020-04-07T22:41:37Z

            u'ꇽꇾꇿꈀꈁꈂꈃꈄꈅꈆꈇꈈꈉ'
        ),
-        'zang': (
+        # zang


We remove the language key? With the key, you could narrow down the failure by logging the language

That's a good point, I can use a tuple here for the test data.

pensivebrian · 2020-04-07T22:42:36Z

-        self.run_charset_validation('zang')
-
-    def run_charset_validation(self, charset):
+    @pytest.mark.parametrize("charset", test_charset_data)


Using pytest, you no longer get integrated tests in vscode. Is there a reason why we're change this?

I don't believe that's true about Pytest anymore (source).

Pytest provides a lot of benefits over unittest, especially in readability of tests. In this particular instance we're using parametrized tests, enabling us to define one test method for multiple tests. This allowed us to remove redundant code.

pytest doesn't work well with parameterized tests. Gene, the engineer before you, ported all pytests to be unittest tests due to this issue. I'd prefer we just leave them as is for now.

That's very interesting—truthfully I've already ported parametrized tests for 4 other test files (interactive, non_interactive, mssqlclient, multiline). Also looks like our original merge uses parametrized tests for ctes and parseutils.

I'll defer to your judgement if you still prefer to remove this, but in my experience it works great.

As long as it easy to debug a particular language, without having to others, I'm okay with it I guess. What's the command line to run pytest with a specific parameter set?

Yes there's still the ability to log which language fails. I'm making a new commit to accommodate this.

I'm not positive on how to call a test from the command line with just a single parameter. This StackOverflow post may show it's possible. I usually comment out the other parameters and call pytest test_mod.py::TestClass::test_method.

So that's what I want to avoid - having to comment out tests to run a test.

I've validated you can test for a single parameter using the -k flag, followed by a string which matches the parameter to test.

ellbosch · 2020-04-09T05:02:52Z

I just added another commit to improve reliability of our tests:

Adds more cleanup that I should have added in the first place for non-interactive tests
Creates test db for some tests that were running off of master db

ellbosch · 2020-04-10T06:16:43Z

Another change today to improve DB create reliability and code quality:

Tests in test_interactive_mode.py no longer use master DB.
A new test_db fixture has been added to mssqltestutils.py to centralize redundant use of create and teardown of test DBs.

pensivebrian

I think the PR is becoming too complicated. I can tell what's stabilization and what is pytest migration. Can we just fix the create database issue, an address pytest some other time?

pensivebrian · 2020-04-10T13:20:28Z

+    """
+    @staticmethod
+    @pytest.fixture(scope='module')
+    def test_db():


creating an iterator for a single element to injecting the client seems overkill. Can we just have a setup method with creates the client?

I'm confused, I don't believe this is an iterator.

We're using Pytest fixtures for tests which were already using this behavior, I was just trying to reduce redundant code.

pensivebrian · 2020-04-10T13:22:51Z

+            assert len(rows) >= min_rows_expected
+            assert len(col) == cols_expected

-if __name__ == u'__main__':


These main's were useful to run files :(

I can put this back!

ellbosch · 2020-04-10T15:00:20Z

I think the PR is becoming too complicated. I can tell what's stabilization and what is pytest migration. Can we just fix the create database issue, an address pytest some other time?

I can simplify this a bit—unfortunately, most of the code changes were about ensuring tests can run off of the master DB (which because a requirement now that we call sys.dm_operation_status).

However, I did refactor things a little in test_special.py, which I can save for another day 🙂

pensivebrian · 2020-04-10T17:34:33Z

-            warnings.warn('Test DB create failed with error: {0}'.format(status))
-        count += 1
-    shutdown(client)
+            if is_create_error:


This will prevent the retry logic from ever running.

pensivebrian · 2020-04-10T17:58:19Z

@@ -122,6 +118,10 @@ def create_test_db():
                    raise AssertionError("Database creation failed: retry logic for SQL " \


Can we surface the status error message in the failure here as well?

pensivebrian

One comment to update the assertion message.

ellbosch · 2020-04-10T22:57:21Z

@pensivebrian I made a small tweak to our helper method to return an error message with the state response in the form of a tuple.

ellbosch requested review from pensivebrian and udeeshagautam March 24, 2020 20:56

pensivebrian requested changes Mar 31, 2020

View reviewed changes

Comment thread tests/mssqltestutils.py Outdated

Comment thread tests/mssqltestutils.py Outdated

pensivebrian self-requested a review March 31, 2020 22:51

pensivebrian requested changes Mar 31, 2020

View reviewed changes

ellbosch requested a review from pensivebrian April 2, 2020 18:01

pensivebrian reviewed Apr 2, 2020

View reviewed changes

Comment thread tests/mssqltestutils.py Outdated

ellbosch requested a review from pensivebrian April 2, 2020 22:59

pensivebrian approved these changes Apr 2, 2020

View reviewed changes

Comment thread tests/mssqltestutils.py Outdated

pensivebrian requested changes Apr 7, 2020

View reviewed changes

Comment thread tests/mssqltestutils.py Outdated

pensivebrian requested changes Apr 7, 2020

View reviewed changes

Comment thread tests/mssqltestutils.py Outdated

pensivebrian reviewed Apr 7, 2020

View reviewed changes

pensivebrian requested changes Apr 7, 2020

View reviewed changes

ellbosch mentioned this pull request Apr 10, 2020

Cast row_limit as integer #448

Merged

pensivebrian requested changes Apr 10, 2020

View reviewed changes

ellbosch force-pushed the ellbosch/stabilize-create-db branch from bbcfc71 to e347b5b Compare April 10, 2020 17:17

pensivebrian reviewed Apr 10, 2020

View reviewed changes

pensivebrian approved these changes Apr 10, 2020

View reviewed changes

Removed py36 from tox

96cc715

ellbosch force-pushed the ellbosch/stabilize-create-db branch from 575e39b to 0ef1d8c Compare April 11, 2020 02:31

ellbosch and others added 14 commits April 10, 2020 19:32

Merge branch 'master' of https://github.com/dbcli/mssql-cli

cea0205

Checks db create--only works on master

22b11e3

Adds check for master database

df8ac38

remove retry logic for create db

b0aec16

improved retry logic

1248e5d

added sleep call

bba3152

added exception raising

8926d69

enforce master db

d21a42f

added on prem check

f0bc165

added support for on-prem

95da004

changes to try/finally

18e41a8

switched order of switch statements

f25b460

added error message

d25ac44

moved scope of break and sleep statements

616829c

ellbosch force-pushed the ellbosch/stabilize-create-db branch from 0ef1d8c to 616829c Compare April 11, 2020 17:44

fixed 'referenced before assignment' bug

d12eaa7

ellbosch merged commit df4246d into master Apr 14, 2020

ellbosch deleted the ellbosch/stabilize-create-db branch April 14, 2020 19:49

ellbosch mentioned this pull request Apr 20, 2020

Special package sometimes raises "Command not found" #446

Closed

		@@ -122,6 +118,10 @@ def create_test_db():
		raise AssertionError("Database creation failed: retry logic for SQL " \

Conversation

ellbosch commented Mar 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pensivebrian left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pensivebrian left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ellbosch commented Apr 4, 2020

Uh oh!

pensivebrian left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pensivebrian left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pensivebrian left a comment

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!

ellbosch commented Apr 9, 2020

Uh oh!

ellbosch commented Apr 10, 2020

Uh oh!

pensivebrian left a comment

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!

ellbosch commented Apr 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pensivebrian left a comment

Choose a reason for hiding this comment

Uh oh!

ellbosch commented Mar 24, 2020 •

edited

Loading