113 Improve reporting of test failures for CQL tests, take two #159

absurdfarce · 2021-06-21T20:20:39Z

No description provided.

python/adelphi/tests/integration/test_cql.py

grighetto · 2021-06-25T11:09:49Z

While this is a great improvement over what we currently have in the new test framework, it's still not as easy to parse as diff was. For example, I changed a single letter in a data type and I got this ouput:

E AssertionError: Items in the first set but not the second:
E "CREATE TABLE IF NOT EXISTS ks_0.tbl_0 ( col_0 ascii, col_1 bigint, col_2 blob, col_3 boolean, col_4 frozen<udt_1>, col_5 map<int, int>, col_6 set, col_7 frozen<tuple<int, frozen<tuple<text, bouble>>>>, col_8 text, col_9 timestamp, col_10 timeuuid, col_11 uuid, col_12 text, col_13 varint, col_14 decimal, col_15 double, col_16 float, col_17 inet, col_18 int, col_19 list, col_20 frozen<udt_2>, PRIMARY KEY ((col_0, col_1), col_2, col_3) ) WITH CLUSTERING ORDER BY (col_2 ASC, col_3 ASC) AND bloom_filter_fp_chance = 0.01 AND caching = {'keys': 'ALL', 'rows_per_partition': 'NONE'} AND comment = '' AND compaction = {'class': 'org.apache.cassandra.db.compaction.SizeTieredCompactionStrategy', 'max_threshold': '32', 'min_threshold': '4'} AND compression = {'enabled': 'false'} AND crc_check_chance = 1.0 AND dclocal_read_repair_chance = 0.1 AND default_time_to_live = 0 AND gc_grace_seconds = 864000 AND max_index_interval = 2048 AND memtable_flush_period_in_ms = 0 AND min_index_interval = 128 AND read_repair_chance = 0.0 AND speculative_retry = '99PERCENTILE';"
E Items in the second set but not the first:
E "CREATE TABLE IF NOT EXISTS ks_0.tbl_0 ( col_0 ascii, col_1 bigint, col_2 blob, col_3 boolean, col_4 frozen<udt_1>, col_5 map<int, int>, col_6 set, col_7 frozen<tuple<int, frozen<tuple<text, double>>>>, col_8 text, col_9 timestamp, col_10 timeuuid, col_11 uuid, col_12 text, col_13 varint, col_14 decimal, col_15 double, col_16 float, col_17 inet, col_18 int, col_19 list, col_20 frozen<udt_2>, PRIMARY KEY ((col_0, col_1), col_2, col_3) ) WITH CLUSTERING ORDER BY (col_2 ASC, col_3 ASC) AND bloom_filter_fp_chance = 0.01 AND caching = {'keys': 'ALL', 'rows_per_partition': 'NONE'} AND comment = '' AND compaction = {'class': 'org.apache.cassandra.db.compaction.SizeTieredCompactionStrategy', 'max_threshold': '32', 'min_threshold': '4'} AND compression = {'enabled': 'false'} AND crc_check_chance = 1.0 AND dclocal_read_repair_chance = 0.1 AND default_time_to_live = 0 AND gc_grace_seconds = 864000 AND max_index_interval = 2048 AND memtable_flush_period_in_ms = 0 AND min_index_interval = 128 AND read_repair_chance = 0.0 AND speculative_retry = '99PERCENTILE';"

What do you think about comparing line by line instead? I think that's a minor change that would help a lot with debugging.

grighetto · 2021-07-12T18:12:31Z

Another issue I'm having is that my tox.ini file doesn't take effect anymore. I used to manually edit it before to set the correct python cli name for my local environment.
I think that was introduced it in #152 and the tox config became hardcoded.

… of lines fed into difflib include a trailing newline to avoid trivial mismatches. Shell redirection won't always give us a trailing newline so we manually make sure it's there.

* Shift to cleanup functions rather than an explicit tearDown * Add a few validation fns to linesWithNewline * Add test-adelphi CLI flag for preserving temp dirs

…e sure we take the keyspaces in order

absurdfarce · 2021-08-18T21:40:20Z

Re: line-by-line diffs... I also wasn't super-happy with the display of diff output, so I finally just knuckled down and switched over to difflib. Given the following change to the 4.0 reference CQL file:

diff --git a/python/adelphi/tests/integration/resources/cql-schemas/4.0.0.cql b/python/adelphi/tests/integration/resources/cql-schemas/4.0.0.cql
index dbe0a97..c875e18 100644
--- a/python/adelphi/tests/integration/resources/cql-schemas/4.0-rc1.cql
+++ b/python/adelphi/tests/integration/resources/cql-schemas/4.0-rc1.cql
@@ -6,7 +6,7 @@ CREATE TYPE IF NOT EXISTS ks_0.udt_0 (
 );
 
 CREATE TYPE IF NOT EXISTS ks_0.udt_1 (
-    fld_2 frozen<udt_2>,
+    fld_22 frozen<udt_2>,
     fld_3 frozen<map<frozen<udt_2>, frozen<udt_2>>>,
     fld_4 frozen<list<frozen<udt_0>>>,
     fld_5 frozen<tuple<frozen<udt_2>, frozen<udt_0>>>

the output on test failure now looks like this:

Diff of generated file (4.0-rc1-stdout.out) against reference file (4.0-rc1.cql)                                                                                                  
--- /tmp/tmp6h5y4g6k/4.0-rc1-stdout.out                                                                                                                                           
+++ /work/git/adelphi/python/adelphi/tests/integration/resources/cql-schemas/4.0-rc1.cql                                                                                          
@@ -6,7 +6,7 @@                                                                                                                                                                   
);
                                      
CREATE TYPE IF NOT EXISTS ks_0.udt_1 (
-    fld_2 frozen<udt_2>,
+    fld_22 frozen<udt_2>,
fld_3 frozen<map<frozen<udt_2>, frozen<udt_2>>>,
fld_4 frozen<list<frozen<udt_0>>>,    
fld_5 frozen<tuple<frozen<udt_2>, frozen<udt_0>>>

absurdfarce · 2021-08-18T21:53:12Z

Re: automated generation of tox.ini... this really should be in control of test-adelphi (since it needs to manipulate this file in order to set Cassandra version as well as several other options). The intent is that whatever features we want to expose for tox.ini would migrate to CLI args of test-adelphi itself... something like this is done in this PR for keeping temp directories around after a run. The big advantage of this approach is that it insulates users from having to know about tox, specifically about how to configure it... the app tries to hide as much of that as possible.

For the moment I've added a CLI flag to disable auto-generation of tox.ini, but going forward I'd much rather identify whatever functionality we want to include there and move it into the CLI rather than managing tox.ini outside of test-adelphi.

absurdfarce · 2021-08-18T21:55:19Z

In addition to the changes addressing the specific comments in the earlier review I've also added a number of improvements/bug fixes discovered via testing. Most notably I've attempted to enforce a deterministic order on keyspaces and tables within keyspace objects themselves (and, by extension, in their generated output). We were seeing cases (especially with Python 2.7) where random ordering was messing with our difflib results in unpleasant ways.

absurdfarce · 2021-08-18T21:56:34Z

python/adelphi/adelphi/export.py

@@ -95,7 +95,8 @@ def make_tuple(ks):
            if props['anonymize']:
                anonymize_keyspace(ks)
            return KsTuple(ids[orig_name], ks)
-        return {t.ks_obj.name : t for t in [make_tuple(ks) for ks in keyspaces]}
+        tuples = sorted([make_tuple(ks) for ks in keyspaces], key=lambda ks: ks.ks_obj.name)
+        return OrderedDict([(t.ks_obj.name,t) for t in tuples])


Keyspaces returned via export should be handled in an orderly way. This is essential for legit compares of prior output to what's done by the current code, which in turn is an essential feature for our integration tests.

absurdfarce · 2021-08-18T21:58:01Z

python/adelphi/adelphi/store.py

+    # Make sure tables are ordered (by table name)
+    for ks_obj in all_keyspace_objs:
+        ks_obj.tables = OrderedDict(sorted(ks_obj.tables.items(), key=lambda item: item[0]))
+


A similar problem to what's discussed above: tables should also follow a coherent order. Tests on Python 2.7 were seeing tables returned in an apparently random order, which in turn caused comparisons to a reference output to fail only because of the ordering of elements within the output.

absurdfarce · 2021-08-18T21:58:31Z

python/adelphi/bin/test-adelphi

@@ -20,7 +20,7 @@ import tox


 # Default C* versions to include in all integration tests
-DEFAULT_CASSANDRA_VERSIONS = ["2.1.22", "2.2.19", "3.0.23", "3.11.10", "4.0-rc1"]
+DEFAULT_CASSANDRA_VERSIONS = ["2.1.22", "2.2.19", "3.0.23", "3.11.10", "4.0.0"]


An update to 4.0.0... since I was already here :)

absurdfarce · 2021-08-18T22:01:23Z

python/adelphi/tests/integration/__init__.py

+        if "KEEP_LOGS" in os.environ:
+            log.info("KEEP_LOGS env var set, preserving logs/output at {}".format(self.dirs.basePath))
+        else:
+            shutil.rmtree(self.dirs.basePath)


Switched to cleanup functions rather than an explicit tearDown() when trying to diagnose a different problem but in the end I decided to keep the change. tearDown() does have some odd semantics; it's not called if setUp() doesn't succeed, for instance. Rather than worry about it in the future it seemed saner to just convert to cleanup methods directly.

absurdfarce · 2021-08-18T22:03:31Z

python/adelphi/tests/integration/test_cql.py

+            cqlStr = "\n\n".join(open(schemas[ks]).read() for ks in sortedKeyspaces)
+            allOutputFile.write(cqlStr)
+
+        return allOutputPath


Another problem observed in testing: the ordering of individual keyspace schemas returned by the glob above isn't deterministic so there's no guarantee we'll get the right ordering of keyspaces when we reconstruct the "all" schema containing everything. To fix this we extract the keyspace name from each individual keyspace schema and traverse them in order when generating the "all" schema. Note that this assumes deterministic output of keyspace names as described above.

This reverts commit be94399.

Logging CQL statements rather than digests when we observe mismatches

3f2efc5

absurdfarce commented Jun 21, 2021

View reviewed changes

python/adelphi/tests/integration/test_cql.py Outdated Show resolved Hide resolved

absurdfarce self-assigned this Jun 21, 2021

absurdfarce requested review from grighetto and jdonenine June 21, 2021 20:53

absurdfarce linked an issue Jun 21, 2021 that may be closed by this pull request

Make reporting/logging for Python integration test infrastructure a bit more sane #113

Open

absurdfarce added 10 commits July 30, 2021 15:47

Move to difflib for outputting differences. Also make sure all arrays…

e5dd14f

… of lines fed into difflib include a trailing newline to avoid trivial mismatches. Shell redirection won't always give us a trailing newline so we manually make sure it's there.

Fix false test failures + clean up the dispaly a bit

14e1d29

Another tweak to diff display format

63e9150

Some minor cleanups

8eff34c

Some other minor cleanup

055ac44

Another minor cleanup

bbb13af

Upgrade C* 4.0-rc1 to 4.0.0

dc61fab

Numerous fixes here:

a3061d1

* Shift to cleanup functions rather than an explicit tearDown * Add a few validation fns to linesWithNewline * Add test-adelphi CLI flag for preserving temp dirs

When compiling per-keyspace outputDir data into a single CQL file mak…

d4cd870

…e sure we take the keyspaces in order

Impose an ordering on tables within keyspace objects as well

5073fed

Add CLI flag to disable generation of tox.ini

be94399

absurdfarce commented Aug 18, 2021

View reviewed changes

Revert "Add CLI flag to disable generation of tox.ini"

2e8e9c9

This reverts commit be94399.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

113 Improve reporting of test failures for CQL tests, take two #159

113 Improve reporting of test failures for CQL tests, take two #159

absurdfarce commented Jun 21, 2021

grighetto commented Jun 25, 2021 •

edited

Loading

grighetto commented Jul 12, 2021

absurdfarce commented Aug 18, 2021

absurdfarce commented Aug 18, 2021

absurdfarce commented Aug 18, 2021

absurdfarce Aug 18, 2021

absurdfarce Aug 18, 2021

absurdfarce Aug 18, 2021

absurdfarce Aug 18, 2021

absurdfarce Aug 18, 2021 •

edited

Loading

113 Improve reporting of test failures for CQL tests, take two #159

Are you sure you want to change the base?

113 Improve reporting of test failures for CQL tests, take two #159

Conversation

absurdfarce commented Jun 21, 2021

grighetto commented Jun 25, 2021 • edited Loading

grighetto commented Jul 12, 2021

absurdfarce commented Aug 18, 2021

absurdfarce commented Aug 18, 2021

absurdfarce commented Aug 18, 2021

absurdfarce Aug 18, 2021

Choose a reason for hiding this comment

absurdfarce Aug 18, 2021

Choose a reason for hiding this comment

absurdfarce Aug 18, 2021

Choose a reason for hiding this comment

absurdfarce Aug 18, 2021

Choose a reason for hiding this comment

absurdfarce Aug 18, 2021 • edited Loading

Choose a reason for hiding this comment

grighetto commented Jun 25, 2021 •

edited

Loading

absurdfarce Aug 18, 2021 •

edited

Loading