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.

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

List test files dynamically #313

Conversation

dev-jonghoonpark
Copy link
Contributor

Related issue: #302

Motivation: Currently we have to manually update the all_tests variable when introducing new test files.

Fix: I've modified it to list test files dynamically, but rather than modify it to add all test files, I've first modified it to only add test files from the following 4 paths so that it doesn't deviate too much from what we already do

  • unit
  • unit/type
  • unit/cluster
  • integration

The result: Previously, 92 tests were in the all_tests variable.
Currently, 93 tests from the four paths mentioned above are in the all_tests variable (unit/type/list-common was added. the rest is the same).

Signed-off-by: jonghoonpark <dev@jonghoonpark.com>
@dev-jonghoonpark dev-jonghoonpark force-pushed the feature/list-test-files-dynamically branch from c6970c1 to 003c2b6 Compare April 12, 2024 14:59
@madolson madolson requested a review from hpatro April 12, 2024 16:23
tests/test_helper.tcl Show resolved Hide resolved
tests/test_helper.tcl Show resolved Hide resolved
@zuiderkwast
Copy link
Contributor

zuiderkwast commented Apr 13, 2024

unit/type/list-common doesn't contain tests. It just defines some variables. I think we should move it out, to tests/helpers/ or whatevever.

@dev-jonghoonpark
Copy link
Contributor Author

unit/type/list-common doesn't contain tests. It just defines some variables. I think we should move it out, to tests/helpers/ or whatevever.

Currently, the unit/type/list-common file is used in three places.

  • tests/unit/keyspace.tcl
  • tests/unit/type/list.tcl
  • tests/unit/type/list-2.tcl

what about move to tests/includes?

@zuiderkwast @madolson

@madolson
Copy link
Member

what about move to tests/includes?

I would just throw it at the bottom of support/util.tcl and remove all the special includes. Right now we just throw alls sorts of random stuff there.

@zuiderkwast
Copy link
Contributor

what about move to tests/includes?

I would just throw it at the bottom of support/util.tcl and remove all the special includes. Right now we just throw alls sorts of random stuff there.

Yes but let's wrap it in a function to return that big array, right? Now it's just creating it when included. Another option is to duplicate those 3(?) lines in all test suites using it.

@dev-jonghoonpark
Copy link
Contributor Author

dev-jonghoonpark commented Apr 15, 2024

After removing list-common.tcl, I created a function in util.tcl.

Can you check the changes?
@madolson

tests/support/util.tcl Outdated Show resolved Hide resolved
tests/support/util.tcl Outdated Show resolved Hide resolved
Signed-off-by: jonghoonpark <dev@jonghoonpark.com>
@dev-jonghoonpark dev-jonghoonpark force-pushed the feature/list-test-files-dynamically branch from 0a1aeb0 to 901efc6 Compare April 15, 2024 01:43
Copy link
Contributor

@hpatro hpatro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@enjoy-binbin enjoy-binbin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI failed:

*** [err]: Is a ziplist encoded Hash promoted on big payload? in tests/unit/type/hash.tcl
Expected '*hashtable*' to equal or match 'Value at:0x7f0edf3d2300 refcount:1 encoding:listpack serializedlength:178 lru:1871839 lru_seconds_idle:0'

@madolson
Copy link
Member

CI failed:

It's probably a test ordering issue. Since the external test re-uses the same cluster, there was probably a config that was set on an earlier test that is interfering with it. You probably juts need to reset the hash-max-listpack-value back to the default value to get the test to pass.

@dev-jonghoonpark
Copy link
Contributor Author

It's probably a test ordering issue. Since the external test re-uses the same cluster, there was probably a config that was set on an earlier test that is interfering with it. You probably juts need to reset the hash-max-listpack-value back to the default value to get the test to pass.

Thanks for the hint. I knew it had something to do with the order, but I wasn't sure where to look.

I'll try some more.

@madolson

@dev-jonghoonpark
Copy link
Contributor Author

dev-jonghoonpark commented Apr 15, 2024

Added a line to reset the hash-max-listpack-value value to 64 (the default value as stated in valkey.conf)

I tested on the forked repository and confirmed that it passes.

image

@madolson

@dev-jonghoonpark dev-jonghoonpark force-pushed the feature/list-test-files-dynamically branch from fbff5a5 to bd34e50 Compare April 15, 2024 07:34
Signed-off-by: jonghoonpark <dev@jonghoonpark.com>
@dev-jonghoonpark dev-jonghoonpark force-pushed the feature/list-test-files-dynamically branch from bd34e50 to ae2f23c Compare April 15, 2024 07:46
tests/unit/type/hash.tcl Show resolved Hide resolved
@zuiderkwast zuiderkwast merged commit c090874 into valkey-io:unstable Apr 15, 2024
14 checks passed
zuiderkwast pushed a commit to zuiderkwast/placeholderkv that referenced this pull request Apr 17, 2024
Motivation: Currently we have to manually update the all_tests variable
when introducing new test files.

Fix: I've modified it to list test files dynamically, but rather than
modify it to add all test files, I've first modified it to only add test
files from the following 4 paths so that it doesn't deviate too much
from what we already do

- unit
- unit/type
- unit/cluster
- integration

Related issue: valkey-io#302

---------

Signed-off-by: jonghoonpark <dev@jonghoonpark.com>
@daniel-house
Copy link
Member

what about move to tests/includes?

I would just throw it at the bottom of support/util.tcl and remove all the special includes. Right now we just throw alls sorts of random stuff there.

Phrases like "just throw" and "random stuff" give me nervous twitches. As far as I can tell, that's how we got server.h.

PatrickJS pushed a commit to PatrickJS/placeholderkv that referenced this pull request Apr 24, 2024
Motivation: Currently we have to manually update the all_tests variable
when introducing new test files.

Fix: I've modified it to list test files dynamically, but rather than
modify it to add all test files, I've first modified it to only add test
files from the following 4 paths so that it doesn't deviate too much
from what we already do

- unit
- unit/type
- unit/cluster
- integration

Related issue: valkey-io#302

---------

Signed-off-by: jonghoonpark <dev@jonghoonpark.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants