github actions: run integration test on cassandra#339
github actions: run integration test on cassandra#339fruch wants to merge 7 commits intoscylladb:masterfrom
Conversation
fruch
force-pushed
the
integration_test_cassandra
branch
from
4a5dea2 to
7aeeef4
Compare
|
we need to skip the serverless tests, cause they are scylla specific |
fruch
force-pushed
the
integration_test_cassandra
branch
from
7aeeef4 to
ac140e1
Compare
| 'user_defined_functions_enabled': True, | ||
| 'scripted_user_defined_functions_enabled': True, | ||
| 'materialized_views_enabled': True, | ||
| 'sasi_indexes_enabled': True, |
There was a problem hiding this comment.
Why do we need to enable sasi_indexes_enabled ?
There was a problem hiding this comment.
I don't have a clue, it's just same as the driver test was doing before for configuring cassandra, but in a new syntax.
|
here's the failing tests:
|
fruch
force-pushed
the
integration_test_cassandra
branch
3 times, most recently
from
fe5d19e to
83dcd61
Compare
fruch
force-pushed
the
integration_test_cassandra
branch
2 times, most recently
from
6343573 to
c68f40c
Compare
|
last failures on cassandra runs: |
fruch
force-pushed
the
integration_test_cassandra
branch
2 times, most recently
from
99a37c9 to
ab2b233
Compare
|
asyncio: libev and asyncore: the last one seems like leftover from the first run session |
fruch
force-pushed
the
integration_test_cassandra
branch
2 times, most recently
from
0a46a34 to
4536002
Compare
|
multiple issues identified one addressed in 4536002 and one in ccm (a by product of the first issue): |
|
now we have 1 failing across the board: and those in some: |
fruch
force-pushed
the
integration_test_cassandra
branch
from
dd65e7a to
374e0c7
Compare
|
left with asyncio, not being too stable: |
|
seems like the only lasting issue is with asyncio, maybe we leave it out from test with cassandra ? |
| - name: Test with scylla - tablets | ||
| run: | | ||
| export EVENT_LOOP_MANAGER=${{ matrix.event_loop_manager }} | ||
| export SCYLLA_VERSION='release:6.0.2' |
There was a problem hiding this comment.
Maybe change it to 6.2.0, it is way faster then 6.0.2
|
|
||
| # TODO: enable after https://github.com/scylladb/python-driver/issues/121 is fixed | ||
| @unittest.skip('Fails on scylla due to the broadcast_rpc_port is None') | ||
| @pytest.mark.skip('Fails on scylla due to the broadcast_rpc_port is None') |
There was a problem hiding this comment.
What is the difference, why did you replace it ?
There was a problem hiding this comment.
as mentioned in the commit message, unittest.skip doesn't skip the setup/teardown code when using pytest as test runner, hence creating the cluster even for skipped tests.
for some of those test it was an issue, since the cassandra couldn't create the cluster.
| use_cluster('test_ip_change', [3], start=True) | ||
|
|
||
| @local | ||
| @scylla_only |
There was a problem hiding this comment.
Can we have some string or comment that will describe reason why it is only supported by scylla ?
There was a problem hiding this comment.
seems like test is setting api_address which isn't legal for cassandra
I'll try fixing the test
There was a problem hiding this comment.
just removing it, works fine of both scylla and cassnadra
| def setup_module(): | ||
| use_multidc({'DC1': {'RC1': 2, 'RC2': 2}, 'DC2': {'RC1': 3}}) | ||
|
|
||
| # cassandra is failing in a weird way: |
There was a problem hiding this comment.
Can you please create an issue of that and link it in a comment
|
|
||
| @greaterthancass21 | ||
| @pytest.mark.xfail(reason='Column name in CREATE INDEX is not quoted. It\'s a bug in driver or in Scylla') | ||
| @xfail_scylla(reason='Column name in CREATE INDEX is not quoted. It\'s a bug in driver or in Scylla') |
There was a problem hiding this comment.
Please create and issue (in the driver repo) for that and reference it here.
There was a problem hiding this comment.
you already adding it in other PR
https://github.com/scylladb/python-driver/pull/363/files#diff-10e4dc66833987dff353dedf3a30e98f8050a7e36b8cc6744d08c0f2552cbe11R1211
it was a scylla issue, that was fixed in 5.2
| cluster.shutdown() | ||
|
|
||
| @local | ||
| @pytest.mark.xfail(reason='AssertionError: \'RAC1\' != \'r1\' - probably a bug in driver or in Scylla') |
There was a problem hiding this comment.
I would prefere to create an issue and investigate what is going on here, why on scylla driver get's different rack name ?
There was a problem hiding this comment.
it's ccm code
scylla default to GossipingPropertyFileSnitch and the logic there uses RAC1
cassandra default to PropertyFileSnitch and the logic default to r1
we can align it on ccm, if you insist
| cluster.shutdown() | ||
|
|
||
|
|
||
| @scylla_only |
There was a problem hiding this comment.
Can we have explanation why it is ran only on scylla ?
There was a problem hiding this comment.
added a comment, basically seems this feature didn't cover cassandra specific queries (i.e. peers_v2)
| def setup_module(): | ||
| use_cluster('rate_limit', [3], start=True) | ||
|
|
||
| @scylla_only |
There was a problem hiding this comment.
Can we have explanation why to run it only on scylla ?
There was a problem hiding this comment.
RateLimitExceededException is a scylla only feature... it's error only scylla generate, and cassandra doesn't
|
|
||
| # TODO: enable after https://github.com/scylladb/python-driver/issues/121 is fixed | ||
| @unittest.skip('Fails on scylla due to the broadcast_rpc_port is None') | ||
| @pytest.mark.skip('Fails on scylla due to the broadcast_rpc_port is None') |
There was a problem hiding this comment.
Why it is None on scylla, is it expected ? If you don't want to look into it, can you please create an issue for it.
There was a problem hiding this comment.
there is an issue for it on the comment, you raise it yourself :)
| self.assertEqual(('', None, None, b''), result[0].t) | ||
| self.assertEqual(('', None, None, b''), s.execute(read)[0].t) | ||
|
|
||
| @scylla_only |
There was a problem hiding this comment.
this test was introduce as part of fixing this issue:
#201
the behavior of scylla and cassandra isn't the same for what covered in this test
@Lorak-mmk might remember the details, as he introduced this test
fruch
force-pushed
the
integration_test_cassandra
branch
from
374e0c7 to
ccb5e03
Compare
fruch
force-pushed
the
integration_test_cassandra
branch
2 times, most recently
from
7aab14c to
48666ad
Compare
|
@fruch, is it ready to be reviewed ? |
yes it's ready |
| - name: Test with cassandra | ||
| run: | | ||
| export EVENT_LOOP_MANAGER=${{ matrix.event_loop_manager }} | ||
| export CASSANDRA_VERSION='4.1.5' |
There was a problem hiding this comment.
cause the the version that was available when I started this effort
| 'sasi_indexes_enabled': True, | ||
| 'transient_replication_enabled': True, | ||
| }) | ||
| elif Version(cassandra_version) >= Version('2.2'): |
There was a problem hiding this comment.
do we ever test earlier than 2.2 versions?
There was a problem hiding this comment.
so far we didn't test cassandra at all, for sure we haven't test anything else but 4.1.5 we are testing now.
to make sure we don't break compitability completly with cassandra introducing tests with one version of cassandra as well
those test can't be run ontop of cassandra and we should mark them in a why we can skip them
cause some of them are missing, we are getting to situations we fail to the the next test session on the github action builders
cassandra 4.1 and up, removed ms/min and so from name of config options, and it need to be specific in the value itself we have few tests using it, and need to be adapted
…n_timeout` seems like this test was fixed in upsteam to ignore `ErrorMessage` type I don't know the exact reason, but since it take us quite some time to sync with upstream, taking this change on it's own it happens only with cassandra (tested with 4.1) Ref: apache@1b335d4
when we use `unittest.skip` on a test function and run via pytest, we still collect the test and run the setup/teardown even the test function would be skipped. when running with cassandra we get into situations where create cluster is also failing for all kind of reasons, which we don't need that cluster.
couldn't identified the root cause of the failures in those tests and they seem to be failing just on asyncio (which is still exprimatal for cassandra), so they are marked as xfail for now
fruch
force-pushed
the
integration_test_cassandra
branch
from
48666ad to
4b47f4d
Compare
4 participants
to make sure we don't break compitability completly with cassandra introducing tests with one version of cassandra as well