Improve test suite to handle external servers better. by yossigo · Pull Request #9033 · redis/redis

yossigo · 2021-06-02T09:37:57Z

This commit revives the improves the ability to run the test suite against
external servers, instead of launching and managing redis-server processes as
part of the test fixture.

This capability existed in the past, using the --host and --port options.
However, it was quite limited and mostly useful when running a specific tests.
Attempting to run larger chunks of the test suite experienced many issues:

Many tests depend on being able to start and control redis-server themselves,
and there's no clear distinction between external server compatible and other
tests.
Cluster mode is not supported (resulting with CROSSSLOT errors).

This PR cleans up many things and makes it possible to run the entire test suite
against an external server. It also provides more fine grained controls to
handle cases where the external server supports a subset of the Redis commands,
limited number of databases, cluster mode, etc.

The tests directory now contains a README.md file that describes how this
works.

This commit also includes additional cleanups and fixes:

Tests can now be tagged.
Tag-based selection is now unified across start_server, tags and test.
More information is provided about skipped or ignored tests.
Repeated patterns in tests have been extracted to common procedures, both at a
global level and on a per-test file basis.
Cleaned up some cases where test setup was based on a previous test executing
(a major anti-pattern that repeats itself in many places).
Cleaned up some cases where test teardown was not part of a test (in the
future we should have dedicated teardown code that executes even when tests
fail).
Fixed some tests that were flaky running on external servers.

Before this commit, this would result with a confusing error message about "too many bind addresses". After this fix it is possible to set an empty bind address which is a valid configuration (any address).

oranagra

i think we need to add a CI job that spins up a server (possibly one in cluster mode) and runs the test suite against it.
this way is a test is added that's incompatible with the external mode, we'll detect it before merging.
if we do that, we'll also feel safer to make the skip-external an opt-out feature rather than the opt-in one (external-ok)

oranagra · 2021-06-02T13:08:52Z

+        # two seconds.
+        after 2000


i think we should use a wait-for-condition here with a timeout rather than an sleep. (that's actually true for all sleeps).

any test that's time-dependent (has a long sleep, or a wait_for that waits for something in redis that takes a long time), should have a slow tag, so that we can run the test suite skipping inherently slow tests

i'd like the slow skip tag to somehow be used by default? (i want it to be an opt-in, like --accurate rather than opt-out), or at the very least, i want to skip these in the CI.yml runs, since we keep adding more and more of these.

@sundb FYI (in the context of #9003)

sundb · 2021-06-02T16:01:00Z

@yossigo I'm not sure if I'm reading this wrong, I seem to remember reading somewhere that we can ignore certain tags when running runtest, but I can't find it now.

oranagra · 2021-06-03T06:31:16Z

@sundb there's the --tags argument (had it since forever), have a look at the new README Yossi added with an example.

sundb · 2021-06-03T07:26:45Z

@oranagra I see that.
i.e. we can use --tags -slow to ignore slow tests.

This reverts commit dd74c23.

oranagra

last round of comments.
p.s. i see that a few of my old ones are still unresolved.

* Skip slow tests on CI * Move external tests (CI and daily) to a separate file.

sundb · 2021-06-09T12:47:33Z

@yossigo I have a problem with my branch merge this pr, when a file has only one test it reports an error.
exec ./runtest --single unit/querybuf --tags -slow.

start_server {tags {slow}} {
    test "query buffer resized correctly" {
    }
}

error

Executing test client: expected non-negative integer but got "".
expected non-negative integer but got ""
    while executing
"read $::test_server_fd $bytes"
    (procedure "test_client_main" line 7)
    invoked from within
"test_client_main $::test_server_port "

When I change it to the following, no error occurs.

start_server {} {
    tags {slow} {
        test "query buffer resized correctly" {

        }
    }
}

oranagra · 2021-06-09T13:07:35Z

@sundb it seems to work for me. even checked out your branch and executed the test with and without --tags -slow.

sundb · 2021-06-09T14:26:10Z

@oranagra It's ok without --tags -slow and --tags slow is also ok.
I ran the same test on unit/type/string, also had this problem, and could not use --clients 1, it will ok.

change string.tcl to follow and run ./runtest --single unit/type/string --tags -slow

start_server {tags {"slow"}} {
    test {Very big payload random access} {
    } {}
}

sundb · 2021-06-09T14:28:22Z

@oranagra My branch is working fine because I moved tags {slow} inside start_server.

sundb · 2021-06-09T14:54:39Z

@oranagra It only goes wrong with --single testname --tags -slow, /runtest --tags -slow is ok.

This commit revives the improves the ability to run the test suite against external servers, instead of launching and managing `redis-server` processes as part of the test fixture. This capability existed in the past, using the `--host` and `--port` options. However, it was quite limited and mostly useful when running a specific tests. Attempting to run larger chunks of the test suite experienced many issues: * Many tests depend on being able to start and control `redis-server` themselves, and there's no clear distinction between external server compatible and other tests. * Cluster mode is not supported (resulting with `CROSSSLOT` errors). This PR cleans up many things and makes it possible to run the entire test suite against an external server. It also provides more fine grained controls to handle cases where the external server supports a subset of the Redis commands, limited number of databases, cluster mode, etc. The tests directory now contains a `README.md` file that describes how this works. This commit also includes additional cleanups and fixes: * Tests can now be tagged. * Tag-based selection is now unified across `start_server`, `tags` and `test`. * More information is provided about skipped or ignored tests. * Repeated patterns in tests have been extracted to common procedures, both at a global level and on a per-test file basis. * Cleaned up some cases where test setup was based on a previous test executing (a major anti-pattern that repeats itself in many places). * Cleaned up some cases where test teardown was not part of a test (in the future we should have dedicated teardown code that executes even when tests fail). * Fixed some tests that were flaky running on external servers.

yossigo added 21 commits May 25, 2021 17:50

Fix CONFIG SET bind with no addresses.

dd74c23

Before this commit, this would result with a confusing error message about "too many bind addresses". After this fix it is possible to set an empty bind address which is a valid configuration (any address).

WIP: Reinstate external server support.

d341bef

Add singledb support.

14c1fa4

Fix broken tests.

abd0639

Add --ignore-encoding to tests.

730be03

Clean up test tagging mechanism.

b42ec22

Add --ignore-digest option.

2404a61

Don't rely on debug populate.

93246ee

Encapsulate RESP2/3 handling.

c2cc1c4

Add needs: tags.

ef0510e

Clean up blocked_clients handling.

83baf00

Improve test stability.

a237597

Improve stability, don't assume default config.

b7c84f7

Move blocked client handling to util.tcl.

dddc6b6

Restore config as part of the test.

6b4a887

Add cluster mode and tagging to multi-key tests.

23bfff3

Improve cluster tests.

aed54ef

Fix flaky test on some slower environments.

3e848f3

Fix flaky test when slowlog itself gets logged.

f689b20

Fix tests broken by tags.

ceaaa23

Add test tags documentation.

e2de4f2

sundb reviewed Jun 2, 2021

View reviewed changes

Comment thread tests/support/util.tcl Outdated

Merge branch 'upstream/unstable'.

24c4d0a

yossigo force-pushed the external-tests branch from da259f0 to 24c4d0a Compare June 2, 2021 12:47

oranagra reviewed Jun 2, 2021

View reviewed changes

Fix scripting cluster compatibility.

279abb8

oranagra mentioned this pull request Jun 2, 2021

Fix the wrong reisze of querybuf #9003

Merged

yossigo added 4 commits June 7, 2021 13:31

Add a clarifying comment.

3a18ce0

Add missing assert.

d6a864f

Consistent tag styles, update docs.

b95c10d

Fix failing non-external tests.

80e04f0

oranagra reviewed Jun 7, 2021

View reviewed changes

Comment thread tests/unit/keyspace.tcl Outdated

yossigo added 2 commits June 7, 2021 15:49

Fix wrong match.

a76e6cb

Add external test suite daily run.

b824bb6

oranagra reviewed Jun 7, 2021

View reviewed changes

Comment thread .github/workflows/external.yml Outdated

yossigo added 6 commits June 7, 2021 17:07

Improve multikey test.

a608e7b

Move external tests to CI.

14d7f0c

Add external tests to daily workflows.

2752ac8

Merge branch 'unstable' into external-tests

6b9653b

Revert "Fix CONFIG SET bind with no addresses."

dfa4cf0

This reverts commit dd74c23.

Temporarily remove bind from config sanity.

9437730

oranagra reviewed Jun 8, 2021

View reviewed changes

Comment thread tests/unit/introspection.tcl Outdated

Comment thread .github/workflows/ci.yml Outdated

Comment thread tests/README.md

Workflow changes.

4d4e5c8

* Skip slow tests on CI * Move external tests (CI and daily) to a separate file.

yossigo force-pushed the external-tests branch from 8167d8b to 4d4e5c8 Compare June 9, 2021 08:40

Skip bind on external tests only.

861a947

yossigo marked this pull request as ready for review June 9, 2021 09:04

oranagra approved these changes Jun 9, 2021

View reviewed changes

yossigo merged commit 8a86bca into redis:unstable Jun 9, 2021

yossigo deleted the external-tests branch June 9, 2021 12:13

oranagra mentioned this pull request Jun 13, 2021

Change return value type for ZPOPMAX/MIN in RESP3 #8981

Merged

oranagra mentioned this pull request Jul 6, 2021

On 32 bit platform, the bit position of GETBIT/SETBIT/BITFIELD/BITCOUNT,BITPOS may overflow #9191

Merged

Uh oh!

Conversation

yossigo commented Jun 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

oranagra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oranagra Jun 2, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sundb commented Jun 2, 2021

Uh oh!

oranagra commented Jun 3, 2021

Uh oh!

sundb commented Jun 3, 2021

Uh oh!

Uh oh!

Uh oh!

oranagra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sundb commented Jun 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oranagra commented Jun 9, 2021

Uh oh!

sundb commented Jun 9, 2021

Uh oh!

sundb commented Jun 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sundb commented Jun 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

yossigo commented Jun 2, 2021 •

edited

Loading

sundb commented Jun 9, 2021 •

edited

Loading

sundb commented Jun 9, 2021 •

edited

Loading