scylla-rust-driver icon indicating copy to clipboard operation
scylla-rust-driver copied to clipboard

Published 20 hours ago • verified publisher icon scylladb

tests: add integration tests for batch statements

Open dimakr opened this issue 9 months ago • 12 comments

Test cases/ideas are inspired by Java driver batch statements tests.

Pre-review checklist

  • [ ] I have split my patch into logically separate commits.
  • [ ] All commit messages clearly explain what they change and why.
  • [ ] I added relevant tests for new features and bug fixes.
  • [ ] All commits compile, pass static checks and pass test.
  • [ ] PR description sums up the changes and reasons why they should be introduced.
  • [ ] I have provided docstrings for the public items that I want to introduce.
  • [ ] I have adjusted the documentation in ./docs/source/.
  • [ ] I added appropriate Fixes: annotations to PR description.

dimakr avatar Feb 11 '25 14:02 dimakr

cargo semver-checks found no API-breaking changes in this PR! 🎉🥳 Checked commit: fc9def4a1908e16df077d59e08dbf8f8317a565a

github-actions[bot] avatar Feb 11 '25 14:02 github-actions[bot]

I intended to open a PR instead of pushing to your branch directly, sorry @dimakr...

wprzytula avatar Feb 11 '25 16:02 wprzytula

@wprzytula I've made the changes as per the comments above

dimakr avatar Feb 11 '25 22:02 dimakr

Turned out that we have a few batch tests in scylla/tests/integration/session.rs test suite. Moved them to the dedicated batch.rs test suite, for which we are creating new test cases in this PR.

dimakr avatar Feb 12 '25 22:02 dimakr

@wprzytula @Lorak-mmk addressed the last comments.

dimakr avatar Feb 13 '25 15:02 dimakr

@wprzytula @Lorak-mmk addressed the last comments.

In commit "tests: add integration tests for batch statements" some tests are still marked with #[ignore] instead of disabling tablets. Maybe you forgot to fix that?

Lorak-mmk avatar Feb 13 '25 22:02 Lorak-mmk

In commit "tests: add integration tests for batch statements" some tests are still marked with #[ignore] instead of disabling tablets. Maybe you forgot to fix that?

@Lorak-mmk the tablets disabling was done as part of "tests: move batch statements tests to batch.rs module" commit. I've adjusted that commit message to clearly mention this.

dimakr avatar Feb 14 '25 10:02 dimakr

@Lorak-mmk the tablets disabling was done as part of "tests: move batch statements tests to batch.rs module" commit. I've adjusted that commit message to clearly mention this.

Is there a reason for doing it in the commit that moves tests, instead of doing it that way from the beginning?

Lorak-mmk avatar Feb 14 '25 11:02 Lorak-mmk

Is there a reason for doing it in the commit that moves tests, instead of doing it that way from the beginning?

There was a bit of mess in the series of commits in this PR from the beginning after a commit from another person was accidentally pushed in (that one is squashed eventually). But to avoid further issues I've made 'disable tablets'-change in the subsequent commit.

dimakr avatar Feb 14 '25 12:02 dimakr

Two things:

  • I don't think you fixed all tests in regards to tablets. In the "Files changed view" I see that test_cas_batch, test_counter_batch, test_fail_logged_batch_with_counter_increment, test_fail_counter_batch_with_non_counter_increment in batch_statement.rs are still ignored.
  • I see that after this PR we have 2 files: batch.rs and batch_statement.rs. Why? What future tests should go into the latter, and what into the former?

Lorak-mmk avatar Feb 14 '25 12:02 Lorak-mmk

Two things:

It was OK, but due to this number of nit related changes one of recent rebases went wrong. Will correct it

dimakr avatar Feb 14 '25 13:02 dimakr

Two things:

It was OK, but due to this number of nit related changes one of recent rebases went wrong. Will correct it

@Lorak-mmk This time should be ok - got rid of batch_statement.rs as per comment https://github.com/scylladb/scylla-rust-driver/pull/1226#discussion_r1951070923

dimakr avatar Feb 14 '25 16:02 dimakr