Closed Totktonada closed 3 years ago
Nice feature, thanks!
My comments:
We can write tags for unit tests inside a C source file and teach test-run to look for tags there. I like this solution because marking with tags will look similar for other types of tests.
Please, don't forget to update a README and describe how to use tagging in test-run.
After merging the feature to test-run we need to mark all that existed tests with tags.
I would add an option that shows all used tags in a whole testsuite (sorted alphabetically of course). This is needed to choose tags for new tests and maintain a set of tags as small as possible. With a huge list of tags, it will unusably practice.
./test-run.py --tags
app
iproto
memtx
replication
sql
vinyl
Question: do we need an option to show tests without tags?
Probably we can drop an option --long
in a next commit.
P.S. For playing with the feature I have marked Tarantool tests (except suites sql, sql-tap, box, replication and vinyl) and pushed to a branch https://github.com/tarantool/tarantool/tree/ligurio/tagging-tests
Thanks for the feedback!
My next actions in short:
I plan to do so at beginning of the next week.
Full answers are below.
We can write tags for unit tests inside a C source file and teach test-run to look for tags there. I like this solution because marking with tags will look similar for other types of tests.
Sure. I placed TODO comments into the source, which describe exactly same idea. It is a bit more complex than current patch (we should support multiline comments, map an executable name in the build dir into a test name in the source dir). So I decided to postpone it until we'll see a need.
Please, don't forget to update a README and describe how to use tagging in test-run.
Good catch. I'll do.
After merging the feature to test-run we need to mark all that existed tests with tags.
Not necessarily: we can do it on demand. My motivation behind implementing the feature was to move gh-xxxx marks from test names to metainformation and keep ability to run all tests related to a particular issue. IOW, for me it is activity of the kind 'provide the feature and see whether developers will use it'.
I would add an option that shows all used tags in a whole testsuite (sorted alphabetically of course). This is needed to choose tags for new tests and maintain a set of tags as small as possible. With a huge list of tags, it will unusably practice.
I like this idea. Just grep -R tags: test
will show them, but with many repetitions. I'll look, whether it is easy to implement and will either do or postpone.
Question: do we need an option to show tests without tags?
If someone want to look across all test and do some code health activity, it is easy to collect all such tests using grep -L tags: test/*/*.test.{lua,sql,py}
. Unlikely it will be used often, so it looks not profitable to include such code and support it.
Probably we can drop an option
--long
in a next commit.
I trying to modify test-run in the backward compatible way for a long time. Sometimes it helps to update from some ancient test-run version (we recently updated test-run in vshard, where it was not updated two years). I would keep test-run side support and possibly move to tags in tarantool itself.
P.S. For playing with the feature I have marked Tarantool tests (except suites sql, sql-tap, box, replication and vinyl) and pushed to a branch https://github.com/tarantool/tarantool/tree/ligurio/tagging-tests
Thanks! I see that many tags that correspond to a test suite name were added. I had some thoughts about this.
If we'll accept it as a rule, it would be a bit annoying to do it consistently and don't forget about this. So, my first idea was to include a test suite name into implicit tags. But we have, say, sql and sql-tap suites and both are about sql. So next idea was to add a default_tags
directive into the suite.ini
file: a list of tags, which implicitly added to each test of the suite. But then I think: one can just run ./test/test-run.py sql
, because test-run filters tests by a substing of <suite>/<test>
name. So maybe there is no much need to add information, which is available through suite/test name, into tags at all.
What do you think?
P.S. For playing with the feature I have marked Tarantool tests (except suites sql, sql-tap, box, replication and vinyl) and pushed to a branch https://github.com/tarantool/tarantool/tree/ligurio/tagging-tests
Thanks! I see that many tags that correspond to a test suite name were added. I had some thoughts about this. If we'll accept it as a rule, it would be a bit annoying to do it consistently and don't forget about this. So, my first idea was to include a test suite name into implicit tags. But we have, say, sql and sql-tap suites and both are about sql. So next idea was to add a default_tags directive into the suite.ini file: a list of tags, which implicitly added to each test of the suite. But then I think: one can just run ./test/test-run.py sql, because test-run filters tests by a substing of
/ name. So maybe there is no much need to add information, which is available through suite/test name, into tags at all.
Personally, I don't like implicit things. It makes everything more complex. Earlier we have placed tests to a set of directories. It was actually directory-based tagging. It was limited a by a small number of tags: app, engine, replication, vinyl, sql and so on. However, tests in replication dir may cover sql or app too.
Now you want to introduce a more flexible tagging mechanism, and tests may have different tags without limitations. You can even come up with a new own tag and add it to a test!
I think it is not a good idea to mix both mechanisms for tagging tests. With file- or testcase-based tagging directory-tagging become useless and we can keep all our tests in a single directory or split them by types and keep only three dirs: unit, tap and interactive.
Updated README with the same information that is present in the PR description.
A bit updated internal naming.
Added ability to show tags collected from existing tests (added the second commit).
Sorry, I have no time to write tests here (that's free time activity), so I only tested it manually.
@ligurio When we look on the question at this angle, it looks meaningful. Okay. But I would not make it a kind of policy at least until we'll have some validation scripts around (like warning if a test in the 'vinyl' directory has no 'vinyl' tag).
Could you add at least a one simple integration test for feature?
Could you add at least a one simple integration test for feature?
If I'll find some spare time for that. I have some ideas how to better structurize such tests, but I'm not ready to spent 1-2 day on experiments ATM. At least, I'll review your pending PRs first :)
I think it is time to deliver it and show what will occur :)
Updated the test-run submodule in tarantool in 2.10.0-beta1-88-g9ac85ace9, 2.8.2-23-gac4097796, 1.10.11-11-g0d904b7a5.
Usage:
test-run will run only those tests, which have at least one of the provided tags.
Show a list of tags:
The tags metainfo should be placed within a first comment of a test file.
Examples:
.lua file:
.sql file:
.py file:
Unsupported features:
Fixes #22