SQL generation scripts for snapshot tests DB #105

amartyashankha · 2018-07-10T21:48:56Z

No description provided.

obi1kenobi · 2018-07-11T03:28:28Z

If you can appease Travis, this is fine by me.

* Adding scripts to create db. * Generated Commands file. * Added OrientDB schema file * Fixed flake8 error. * Ensure unique parent sets * Addressing comments * Addressing comments * Addressing comments * Addressing comments * Using six.iteritems * Added conftest DB generation * Added snapshot tests * Added conftest file * Generated snapshots file * Adding empty parameters to test inputs * Fixed commands import * Adding parametrized test case * Modifying filter on optional test case * Adding newlines to commands, and checking for comments * Adding outputs to filter_on_optional_variable * Deterministic order of keys * Deterministic order of keys * Adding noqa to snapshots * Adding pyorient to dev requirements * Adding snapshottest to dev requirements * Fixing pylint error * Fixing dev requirements * Adding orientdb install script * Updating snapshots file * Adding compiler version instead of git version. * Turning list of OrientDB results into set of frozensets. * Turning list of OrientDB results into frozenset of Counter of frozensets. * Using docker-compose to set up orientdb. * Using docker-compose to set up orientdb. * Detach docker-compose in travis * Fixing lint errors * Updating travis.yml to ignore snapshot files for flake8. * Converting lists to tuples in query results * Addressing comments * Fixing docker-compose * Fixing isort * Fixing more lint errors * Capitalizing OrientDB * Fixing more lint errors * Moved sample parameters to snapshot testing file * Addressing comments * Trying pylint fix * pylint fix * Fixed typo.

coveralls · 2018-07-16T20:21:19Z

Coverage decreased (-0.03%) to 94.793% when pulling e47ed53 on feat_snapshot_tests into cc373ce on master.

obi1kenobi · 2018-07-17T05:52:48Z

.coveragerc

@@ -1,6 +1,14 @@
 [run]
 # We don't want to include the tests themselves in the coverage report


This comment is outdated now, since more than one thing is omitted.

obi1kenobi · 2018-07-17T05:52:56Z

.travis.yml

@@ -4,16 +4,26 @@ cache: pip
 python:
  - "2.7"
  - "3.6"
+services:
+    - docker


bad indentation

obi1kenobi · 2018-07-17T05:53:57Z

graphql_compiler/scripts/__init__.py

@@ -0,0 +1 @@
+# Copyright 2017-present Kensho Technologies, LLC.


scripts is not a package and shouldn't have an __init__ file

obi1kenobi · 2018-07-17T05:55:46Z

graphql_compiler/scripts/generate_test_sql/__init__.py

+import random
+import sys
+
+from ... import __version__


Since scripts isn't supposed to be a package, you might have to read the version "manually" -- read the file then regex out the __version__ variable's value. The setup.py already does this, follow its lead.

obi1kenobi · 2018-07-17T05:57:01Z

graphql_compiler/scripts/generate_test_sql/utils.py

+    return base_name + SEPARATOR + label
+
+
+def strip_index_from_name(name):


Maybe try to find a better name for this function? The name doesn't seem to match the return value.

obi1kenobi · 2018-07-17T05:57:44Z

graphql_compiler/scripts/generate_test_sql/__init__.py

@@ -0,0 +1,37 @@
+# Copyright 2017-present Kensho Technologies, LLC.


obi1kenobi · 2018-07-17T05:58:35Z

graphql_compiler/tests/conftest.py

@@ -0,0 +1,39 @@
+# Copyright 2018-present Kensho Technologies, LLC.
+


consistency: the other files have the first import immediately below the copyright line, but this one has a blank line in between

obi1kenobi · 2018-07-17T05:59:23Z

graphql_compiler/tests/conftest.py

+            set_up_successfully = True
+            break
+        except Exception:  # pylint: disable=broad-except
+            time.sleep(1)


log the exception

obi1kenobi · 2018-07-17T06:03:29Z

Also, there's a warning in the test logs that you should address:

$ docker-compose up -d
WARNING: Some services (orientdb) use the 'deploy' key, which will be ignored. Compose does not support 'deploy' configuration - use `docker stack deploy` to deploy to a swarm.

I think you just need to remove the deploy key entirely.

obi1kenobi · 2018-07-17T16:51:41Z

graphql_compiler/scripts/generate_test_sql/__init__.py

+    # intentionally *not* adding an encoding option to open
+    # see here:
+    # https://github.com/pypa/virtualenv/issues/201#issuecomment-3145690
+    top_level_directory = os.path.dirname(os.getcwd())


cwd isn't constant, so this code is brittle -- use __file__ because this file's location is fixed, then make it relative to that

obi1kenobi · 2018-07-17T16:52:12Z

graphql_compiler/scripts/generate_test_sql/__init__.py

+
+
+def read_file(filename):
+    """Read package file as text to get its version"""


there's no version checking going on in this function

Didn't quite get this comment.

read_file has a docstring that says it gets a version -- no such thing happens in this function, and what the caller uses this function for is the caller's business. I think the docstring should be updated.

obi1kenobi · 2018-07-17T16:52:58Z

graphql_compiler/tests/test_data_tools/data_tool.py

@@ -1,12 +1,12 @@
 # Copyright 2018-present Kensho Technologies, LLC.
 from glob import glob
-from os import path
+from os import path, getcwd


you don't seem to be using getcwd

obi1kenobi · 2018-07-17T18:35:25Z

.coveragerc

+    graphql_compiler/tests/*
+
+    # No coverage for SQL command generation script
+    graphql_compiler/scripts/generate_test_sql/*


This path doesn't exist anymore.

obi1kenobi · 2018-07-17T19:05:58Z

I think #105 (comment) is the last thing that needs to be addressed before we merge.

amartyashankha and others added 4 commits July 12, 2018 11:48

Fixing snapshots and compiler output

00d7eed

Adding copyright lines

17272df

Changed commands file

7a7a00b

amartyashankha force-pushed the feat_snapshot_tests branch from be15d9d to 7a7a00b Compare July 12, 2018 15:58

Amartya Shankha Biswas added 2 commits July 13, 2018 10:51

Adding docker-compose down to travis

163bc60

Adding docker-compose commands to travis

7e4239d

obi1kenobi reviewed Jul 17, 2018

View reviewed changes

Amartya Shankha Biswas added 6 commits July 17, 2018 11:48

changing coveragerc

293293a

Fixing travis and docker-compose

e796892

Fixed function name

12804be

FIxing copyright

79af4d4

Adding version reading to generate_test_sql

1d0cc41

Renamed data loading file and function.

8a3613f

obi1kenobi reviewed Jul 17, 2018

View reviewed changes

Amartya Shankha Biswas added 4 commits July 17, 2018 13:00

Adding error message on DB setup.

1f378d5

Merging duplicate scripts directories.

a7fcee2

Fixing lint errors

0fa0917

Removing getcwd

675dfbc

obi1kenobi reviewed Jul 17, 2018

View reviewed changes

Amartya Shankha Biswas added 2 commits July 17, 2018 14:46

Removing sinatra XD

387ef44

Updated coveragerc

7c1f807

obi1kenobi previously approved these changes Jul 17, 2018

View reviewed changes

Addressing comments

434f3c2

amartyashankha dismissed obi1kenobi’s stale review via 434f3c2 July 17, 2018 19:10

amartyashankha force-pushed the feat_snapshot_tests branch from f2e85ef to 434f3c2 Compare July 17, 2018 19:10

Merge branch 'master' into feat_snapshot_tests

e47ed53

obi1kenobi approved these changes Jul 17, 2018

View reviewed changes

obi1kenobi merged commit 71d485c into master Jul 17, 2018

obi1kenobi deleted the feat_snapshot_tests branch July 17, 2018 19:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SQL generation scripts for snapshot tests DB #105

SQL generation scripts for snapshot tests DB #105

amartyashankha commented Jul 10, 2018 •

edited

obi1kenobi commented Jul 11, 2018

coveralls commented Jul 16, 2018 •

edited

obi1kenobi Jul 17, 2018

obi1kenobi Jul 17, 2018

obi1kenobi Jul 17, 2018

obi1kenobi Jul 17, 2018

obi1kenobi Jul 17, 2018

obi1kenobi Jul 17, 2018

obi1kenobi Jul 17, 2018

obi1kenobi Jul 17, 2018

obi1kenobi commented Jul 17, 2018 •

edited

obi1kenobi Jul 17, 2018

obi1kenobi Jul 17, 2018

amartyashankha Jul 17, 2018

obi1kenobi Jul 17, 2018

obi1kenobi Jul 17, 2018

obi1kenobi Jul 17, 2018

obi1kenobi commented Jul 17, 2018

		@@ -1,6 +1,14 @@
		[run]
		# We don't want to include the tests themselves in the coverage report

		@@ -0,0 +1 @@
		# Copyright 2017-present Kensho Technologies, LLC.

		return base_name + SEPARATOR + label


		def strip_index_from_name(name):

		@@ -0,0 +1,37 @@
		# Copyright 2017-present Kensho Technologies, LLC.

		@@ -0,0 +1,39 @@
		# Copyright 2018-present Kensho Technologies, LLC.



		def read_file(filename):
		"""Read package file as text to get its version"""

SQL generation scripts for snapshot tests DB #105

SQL generation scripts for snapshot tests DB #105

Conversation

amartyashankha commented Jul 10, 2018 • edited

obi1kenobi commented Jul 11, 2018

coveralls commented Jul 16, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obi1kenobi commented Jul 17, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obi1kenobi commented Jul 17, 2018

amartyashankha commented Jul 10, 2018 •

edited

coveralls commented Jul 16, 2018 •

edited

obi1kenobi commented Jul 17, 2018 •

edited