test/cql-pytest: Add test for token() filter againts mutation_fragmen… #18759
Conversation
| @@ -501,3 +501,11 @@ def test_many_partitions(cql, test_keyspace, scylla_only): | |||
| assert partition_starts == num_partitions | |||
| assert partition_ends == num_partitions | |||
| assert len(pks) == num_partitions | |||
|
|
|||
|
|
|||
| def test_mutation_fragments_vs_token(cql, test_keyspace, scylla_only): | |||
There was a problem hiding this comment.
Please add a comment here - not just in the commit message - what this test is about, and that it reproduces issue #18637. Please explain that prior to that issue being fixed it used to crash, and why we consider the weird error this test expects to be correct (?). If it's not correct, please consider saying that in the comment - or perhaps even better, open an issue on what's not correct and make this test "xfail" with that issue.
Note that even an "xfail" test still runs every time, so it guarantees we don't regress the crash.
There was a problem hiding this comment.
OK, the #18768 is there and the test is updated.
This xfail marking is not extremely nice, since it would "pass" the test even if it fails somewhere else, not in the place that the test author thinks the test should step on. But chances are low indeed, in 99.(9)% of runs it should fail in the expected select
There was a problem hiding this comment.
I don't understand the resistance to writing comments. It really makes me sad. Yes, I know you believe that the function name "test_mutation_fragments_vs_token" is super-self-explanatory. I know you think that the xfail "reason" text will forever stay there and refer to the issue which serves as a reasonable description of what this test is about.
But five years from now, long after this issue is solved and the xfail is removed, I don't believe anyone will understand why this test exists, what it tests or why, or if it fails, which issue regressed and which patch we forgot to backport. Especially since historically we had two kinds of failures here - a crash, and a clean failure, each has its own issue. Wouldn't it be nice to get an explanation of all of this as a reason-for this test which right now looks completely mysterious (because it basically calls select and doesn't even check the results - which looks silly if you don't know what kind of bugs this was meant to reproduce).
There was a problem hiding this comment.
I'm sorry that it made you sad. It's not a resistance to write comments, it's some reluctance to duplicate information 🤷 I can imagine the way to look for information about this particular place -- it's git show -> PR number -> github PR page -> github issue page. It's a lot of information already, even though it takes some effort to dig one. I'll try to be more copy-n-past-y next time
There was a problem hiding this comment.
Yes, it turns out that our decision to put a lot of effort into our git commit log (this is actually a practice that I learned from Avi, in the Linux kernel world) isn't a victimless crime - the main victim is the comments. People begin to feel that comments are not needed, because someone can always use "git blame" to figure the reason for everything. The problem that this isn't true. The oldest cql-pytest test (in test_keyspace.py) test will soon be four year olds. Our oldest dtest tests are probably 10 year old. Sometimes, "git blame" is very helpful. But very often in old code, it begins a long archeological dig to find the original patch that introduced this test and why it was introduced. When code moves or experiences style changes (e.g., take a lot at dtest's git blame) it's even harder to use git blame, and it's hardest to understand how and why the test was changed later, and is it really testing the same thing as the first version was, or maybe something changed in the purpose of this test.
When I see a test failing, I want to know what this test was intended to check - not just what the code is doing - because it's possible the test itself is buggy or checking things in a bad way. Also, if I see a recent test failing on a certain branch (e.g., 5.2), I want to know what issue this test reproduces - to know what we forgot to backport to that certain branch. When I am worried that a certain corner case isn't being tested, I want to read comments about the intentions of the author of the test - whether they forgot to check this case or there was some logic that meant we don't need to test it. Or maybe it is being tested, just in a way I wouldn't recognize just from the code. When I want to see how a certain area of the code (e.g., filter expressions in Alternator) were tested, I want to skim through the comments to understand what was tested, and what wasn't.
Many of these things I tried to push in the cql-pytest came from my past experience with dtest, where I often encountered failing tests and had no idea what they were checking and why. Conversely, in some rare cases where dtest did contain comments, those were often useful to understand failures.
In my opinion, the state of the current code is more important than any of the other things you described (git log or github issues or PRs). It's not that hard to keep both, especially since it's usually not a simple copy-paste. In your test's case I asked that you at the very minimum just say # Reproduces #18637 (I'd prefer more...). Yes, these two words also appear in the commit message and in the "xfail" line. But each of these serve a different purpose.
…ts() When selecting from mutation_fragments(table) one may want to apply token() filtering againts partition key. This doesn't work currently, but used to crash. This patch adds a regression test for that refs: scylladb#18637 Signed-off-by: Pavel Emelyanov <xemul@scylladb.com>
xemul
force-pushed
the
br-match-token-from-mutation-fragments-test
branch
from
ab61bde
to
25db6ed
Compare
🟢 CI State: SUCCESS✅ - Build Build Details:
|
|
@nyh , please re-review |
| @@ -501,3 +501,11 @@ def test_many_partitions(cql, test_keyspace, scylla_only): | |||
| assert partition_starts == num_partitions | |||
| assert partition_ends == num_partitions | |||
| assert len(pks) == num_partitions | |||
|
|
|||
|
|
|||
| def test_mutation_fragments_vs_token(cql, test_keyspace, scylla_only): | |||
There was a problem hiding this comment.
I don't understand the resistance to writing comments. It really makes me sad. Yes, I know you believe that the function name "test_mutation_fragments_vs_token" is super-self-explanatory. I know you think that the xfail "reason" text will forever stay there and refer to the issue which serves as a reasonable description of what this test is about.
But five years from now, long after this issue is solved and the xfail is removed, I don't believe anyone will understand why this test exists, what it tests or why, or if it fails, which issue regressed and which patch we forgot to backport. Especially since historically we had two kinds of failures here - a crash, and a clean failure, each has its own issue. Wouldn't it be nice to get an explanation of all of this as a reason-for this test which right now looks completely mysterious (because it basically calls select and doesn't even check the results - which looks silly if you don't know what kind of bugs this was meant to reproduce).
…ts()
When selecting from mutation_fragments(table) one may want to apply token() filtering againts partition key. This doesn't work currently, but used to crash. This patch adds a regression test for that
refs: #18637