Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Some tests in test/cql-pytest/test_permissions.py fail on Cassandra #15969

Closed
nyh opened this issue Nov 6, 2023 · 5 comments · Fixed by #15979
Closed

Some tests in test/cql-pytest/test_permissions.py fail on Cassandra #15969

nyh opened this issue Nov 6, 2023 · 5 comments · Fixed by #15979
Assignees
@nyh
Labels
area/test Issues related to the testing system code and environment area/udf User-Defined Functions and Aggregates
Milestone
6.0

Comments

@nyh
Copy link
Contributor

nyh commented Nov 6, 2023

Some tests in test/cql-pytest/test_permissions.py fail on Cassandra (I used Cassandra 4.1.1), i.e., the command

test/cql-pytest/run-cassandra test_permissions.py

Two tests fail:

  1. test_drop_udf_with_same_name fails in line 216, trying to use a body1 "(i int) CALLED ON NULL INPUT RETURNS bigint LANGUAGE java AS 'return 42;'" says: Java source compilation failed: Line 1: Type mismatch: cannot convert from int to Long. I guess Java when used in UDF can't promote integer types. How did this ever work before?

  2. test_grant_perms_on_nonexistent_udf fails in line 321, saying "cassandra.protocol.ConfigurationException Keyspace ks doesn't exist". The error message is expected, but the test expects InvalidRequest instead of ConfugrationException. The test should probably be changed to allow both - this is unfortunately a common difference between Scylla and Cassandra and we usually just allow both in affected tests.
@nyh nyh added the area/test Issues related to the testing system code and environment label Nov 6, 2023
@nyh nyh self-assigned this Nov 6, 2023
@nyh
Copy link
Contributor Author

nyh commented Nov 6, 2023

Unfortunately, fixing the above errors reveals the next two errors:

  1. In test_drop_udf_with_same_name, trying to drop an overloaded function without explicit parameters is expected to yield an error, rather than attempting to drop one of them (and failing on "Unauthorized" method because of lack of permissions). Apparently this doesn't happen in Casandra, which seems to try to drop one of the functions of the same name, and fail on Unauthorized. We should consider this should be considered a cassandra_bug, a bug in Scylla which should be reported, or a difference that should be excused by accepting either InvalidRequest or Unauthorized.
  2. In test_grant_perms_on_nonexistent_udf, we try RANT EXECUTE ON FUNCTION ks.fun42(int) TO cql_test_1699286053351 when neither "ks" nor "fun42" exist. Scylla reasonably throws InvalidRequest, Cassandra throws a SyntaxException saying "NoSuchElementException No value present" which doesn't make sense. I think this is a cassandra_bug.

@mykaul mykaul added the area/udf User-Defined Functions and Aggregates label Nov 6, 2023
nyh added a commit to nyh/scylla that referenced this issue Nov 7, 2023
@nyh
test/cql-pytest: fix test_permissions.py to not fail on Cassandra
6cd68f0 
We shouldn't have cql-pytest tests that report failure when run on
Cassandra (with test/cql-pytest/run-cassandra): A test that passes
on Scylla but fails on Cassandra indicates a *difference* between
Scylla's behavior and Cassandra's, and this difference should always
be investigated:

 1. It can be a Scylla bug, which of should be fixed immediately
    or reported as a bug and the test changed to fail on Scylla ("xfail").

 2. It can be a minor difference in Scylla's and Cassandra's
    behavior where both can be accepted. In this case the test should
    me modified to accept both behaviors, and a comment added to
    explain why we decided to do that.

 3. It can be a Cassandra bug which causes a correct test to fail.
    This case should not be taken lightly, and a serious effort
    is needed to be convinced that this is really a Cassandra bug
    and not our misunderstanding of what Cassandra does. In
    this case the test should be marked "cassandra_bug" and a
    detailed comment should explain why.

 4. Or it can be an outright bug in the test that caused it to fail
    on Cassandra.

This test had most of these cases :-) There was a test bug in one place
(in a Cassandra-specific Java UDF), a minor and (aruably) acceptable
difference between the error codes returned by Scylla and Cassandra
in one case, and two minor Cassandra bugs (in the error path). All
of these are fixed here, and after this patch test/cql-pytest/run-cassandra
no longer fails on this file.

Fixes scylladb#15969

Signed-off-by: Nadav Har'El <nyh@scylladb.com>
@nyh nyh mentioned this issue Nov 7, 2023
Merged
@nyh
Copy link
Contributor Author

nyh commented Nov 7, 2023

I sent a pull request to fix this. No backports will be needed - the fixes are only relevant when running the tests against Cassandra, something we don't do in CI, and don't do on old branches.

@mykaul
Copy link
Contributor

mykaul commented Nov 7, 2023

I sent a pull request to fix this. No backports will be needed - the fixes are only relevant when running the tests against Cassandra, something we don't do in CI, and don't do on old branches.

I think we do test against Cassandra with our Python driver test matrix (still doesn't mean we'll need backports though? unsure).

@mykaul mykaul added this to the 6.0 milestone Nov 7, 2023
@nyh
Copy link
Contributor Author

nyh commented Nov 7, 2023

I think we do test against Cassandra with our Python driver test matrix (still doesn't mean we'll need backports though? unsure).

Thisissue is about a scylla.git test (in test/cql-pytest), not a Python-driver test.
These may have some (fairly small) overlaps, but are not the same. The functionality that a Python driver test should cover is very different from what Scylla's test covers - in this case, we test "DROP FUNCTION ..." on various error paths (missing keyspace, missing function, missing argument types, etc.) because we want to check them in Scylla, but as far as the Python driver is concerned, they are all just a request passed as text to the server.

nyh added a commit to nyh/scylla that referenced this issue Nov 7, 2023
@nyh
test/cql-pytest: fix test_permissions.py to not fail on Cassandra
baaa94b 
We shouldn't have cql-pytest tests that report failure when run on
Cassandra (with test/cql-pytest/run-cassandra): A test that passes
on Scylla but fails on Cassandra indicates a *difference* between
Scylla's behavior and Cassandra's, and this difference should always
be investigated:

 1. It can be a Scylla bug, which of should be fixed immediately
    or reported as a bug and the test changed to fail on Scylla ("xfail").

 2. It can be a minor difference in Scylla's and Cassandra's
    behavior where both can be accepted. In this case the test should
    me modified to accept both behaviors, and a comment added to
    explain why we decided to do that.

 3. It can be a Cassandra bug which causes a correct test to fail.
    This case should not be taken lightly, and a serious effort
    is needed to be convinced that this is really a Cassandra bug
    and not our misunderstanding of what Cassandra does. In
    this case the test should be marked "cassandra_bug" and a
    detailed comment should explain why.

 4. Or it can be an outright bug in the test that caused it to fail
    on Cassandra.

This test had most of these cases :-) There was a test bug in one place
(in a Cassandra-specific Java UDF), a minor and (aruably) acceptable
difference between the error codes returned by Scylla and Cassandra
in one case, and two minor Cassandra bugs (in the error path). All
of these are fixed here, and after this patch test/cql-pytest/run-cassandra
no longer fails on this file.

Fixes scylladb#15969

Signed-off-by: Nadav Har'El <nyh@scylladb.com>
@nyh nyh mentioned this issue Nov 12, 2023
Closed
denesb added a commit that referenced this issue Nov 14, 2023
@denesb
Merge 'test/cql-pytest: fix test_permissions.py to not fail on Cassan…
f63645c 
…dra' from Nadav Har'El

This short series fixes test/cql-pytest/test_permissions.py to stop failing on Cassandra.

The second patch fixes these failures (and explains why). The first patch is a new test for UDFs, which helped me prove that one of the test_permissions.py failures in Cassandra is a Cassandra bug - some esoteric error path that prints the right message when no permissions are involved, becomes wrong when permissions are added.

Fixes #15969

Closes #15979

* github.com:scylladb/scylladb:
  test/cql-pytest: fix test_permissions.py to not fail on Cassandra
  test/cql-pytest: add test for DROP FUNCTION
@denesb
Copy link
Contributor

denesb commented Dec 18, 2023

Only affects running cql-pytest against C*, not backporting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@nyh nyh

Labels
area/test Issues related to the testing system code and environment area/udf User-Defined Functions and Aggregates
Projects
None yet
Milestone
6.0
Development

Successfully merging a pull request may close this issue.

test/cql-pytest: fix test_permissions.py to not fail on Cassandra nyh/scylla
4 participants
@nyh @denesb @mykaul @scylladb-promoter