Support tags #314

Totktonada · 2021-10-02T00:10:47Z

Usage:

./test-run.py --tags foo
./test-run.py --tags foo,bar app/ app-tap/

test-run will run only those tests, which have at least one of the
provided tags.

Show a list of tags:

./test-run.py --tags
./test-run.py app-tap/ --tags

The tags metainfo should be placed within a first comment of a test
file.

Examples:

.lua file:

#!/usr/bin/tarantool

-- tags: foo, bar
-- tags: one, more

<...>

.sql file:
```
-- tags: foo
-- tags: bar
<...>
```
.py file:
```
# tags: foo

<...>
```

Unsupported features:

Marking unit tests with tags.
Multiline comments (use singleline ones for now).

Fixes #22

ligurio · 2021-10-02T10:26:12Z

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

Totktonada · 2021-10-02T13:01:36Z

Thanks for the feedback!

My next actions in short:

I'll update README.
I'll look whether it is easy to print all tags and will either implement or postpone.

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?

ligurio · 2021-10-04T19:01:46Z

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.

Totktonada · 2021-10-05T18:45:31Z

Updated README with the same information that is present in the PR description.

A bit updated internal naming.

Diff

diff --git a/lib/server.py b/lib/server.py
index f62612c..f56bc9f 100644
--- a/lib/server.py
+++ b/lib/server.py
@@ -172,13 +172,13 @@ class Server(object):
         # TODO: Support multiline comments (mainly for unit
         # tests).
 
-        def match_any(test_name, patterns):
+        def match_any_pattern(test_name, patterns):
             for pattern in patterns:
                 if pattern in test_name:
                     return True
             return False
 
-        def match_tags(test_name, accepted_tags):
+        def match_any_tag(test_name, accepted_tags):
             tags = find_tags(test_name)
             for tag in tags:
                 if tag in accepted_tags:
@@ -189,8 +189,8 @@ class Server(object):
 
         res = []
         for test_name in test_names:
-            if match_any(test_name, exclude_patterns):
+            if match_any_pattern(test_name, exclude_patterns):
                 continue
-            if accepted_tags is None or match_tags(test_name, accepted_tags):
+            if accepted_tags is None or match_any_tag(test_name, accepted_tags):
                 res.append(test_name)
         return res

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.

Totktonada · 2021-10-05T18:53:23Z

@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).

lib/options.py

Usage: ```sh ./test-run.py --tags foo ./test-run.py --tags foo,bar app/ app-tap/ ``` test-run will run only those tests, which have at least one of the provided tags. The tags metainfo should be placed within a first comment of a test file (see examples in the README.md file). Unsupported features: * Marking unit tests with tags. * Multiline comments (use singleline ones for now). Fixes #22

Usage: ```sh ./test-run.py --tags ./test-run.py app-tap/ --tags ``` Follows up #22

VitaliyaIoffe · 2021-10-14T08:43:51Z

lib/utils.py

+            line = line.rstrip('\n')
+            if line.startswith('#!'):
+                pass
+            elif line == '':
+                pass


Is not the possible situation when a line starts with several spaces?
Maybe for clearness use line = line.strip()

So, it would like the following:

#!/usr/bin/env tarantool -- tags: lalala -- -- Blah blah.

Or:

#!/usr/bin/env tarantool -- Blah blah. -- -- tags: lalala

It would be quite strange to indent a top level comment this way. Since we accept only first comment (precisely: first non-empty block except maybe shebang, when it is a comment), it must be at the top level.

Another possibility is the 'empty' line that contains only whitespaces. Trailing whitespaces are highly discouraged in programming, even git diff colors them as red.

So, back to your question: yep, it is technically possible, but has no sense.

VitaliyaIoffe · 2021-10-14T09:00:21Z

lib/utils.py

+    """ Extract tags from a first comment in the file.
+    """


This remark is not associated with your pull request, but with all docstrings through the project. In spite of citing PEP257 our docstrings have no identical format. I think we have to change this and fix the format for one-line and multi-line docstrings in our code style.

I prefer the way shown here, because of several reasons:

The text is indented by four spaces.

Short single line descriptions and long multiline ones look uniformly.

Ending a single line comment with a whitespace (to make it centric) seems a bit unnatural for me (""" Foo. """).

I'm trying to stick with this style.

I don't know, to be honest, what is the best, but I would not really bother much. Only if it is 10% of your average bi-weekly work around the project (IOW, when you work on it almost all time). Otherwise it is too costly.

I didn't mean to refactor the code. I mean fix one type of code syle in our developer guide.

ligurio · 2021-10-14T10:18:11Z

Could you add at least a one simple integration test for feature?

Totktonada · 2021-10-14T12:46:12Z

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 :)

Totktonada · 2021-10-15T00:38:39Z

I think it is time to deliver it and show what will occur :)

Now it is possible to mark test files with tags and run some group of tests using the `--tags` option. Usage: | ./test-run.py --tags foo | ./test-run.py --tags foo,bar app/ app-tap/ Show the list of tags: | ./test-run.py --tags | ./test-run.py app-tap/ --tags See syntax and more details in tarantool/test-run#314 See the task description and possible usages in tarantool/test-run#22

Now it is possible to mark test files with tags and run some group of tests using the `--tags` option. Usage: | ./test-run.py --tags foo | ./test-run.py --tags foo,bar app/ app-tap/ Show the list of tags: | ./test-run.py --tags | ./test-run.py app-tap/ --tags See syntax and more details in tarantool/test-run#314 See the task description and possible usages in tarantool/test-run#22 (cherry picked from commit 9ac85ac)

Totktonada · 2021-10-15T00:47:56Z

Updated the test-run submodule in tarantool in 2.10.0-beta1-88-g9ac85ace9, 2.8.2-23-gac4097796, 1.10.11-11-g0d904b7a5.

Totktonada requested review from ylobankov and VitaliyaIoffe October 2, 2021 00:10

Totktonada force-pushed the Totktonada/gh-22-tags branch from 2d04969 to e030ba2 Compare October 5, 2021 18:41

ylobankov approved these changes Oct 6, 2021

View reviewed changes

lib/options.py Outdated Show resolved Hide resolved

Totktonada added 2 commits October 7, 2021 00:50

Add ability to show a list of tags

5dc30ae

Usage: ```sh ./test-run.py --tags ./test-run.py app-tap/ --tags ``` Follows up #22

Totktonada force-pushed the Totktonada/gh-22-tags branch from e030ba2 to 5dc30ae Compare October 6, 2021 21:51

VitaliyaIoffe reviewed Oct 14, 2021

View reviewed changes

VitaliyaIoffe approved these changes Oct 14, 2021

View reviewed changes

Totktonada merged commit 54f785d into master Oct 15, 2021

Totktonada deleted the Totktonada/gh-22-tags branch October 15, 2021 00:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Support tags #314

Support tags #314

Uh oh!

Totktonada commented Oct 2, 2021 •

edited

Loading

Uh oh!

ligurio commented Oct 2, 2021

Uh oh!

Totktonada commented Oct 2, 2021

Uh oh!

ligurio commented Oct 4, 2021

Uh oh!

Totktonada commented Oct 5, 2021

Uh oh!

Totktonada commented Oct 5, 2021

Uh oh!

Uh oh!

VitaliyaIoffe Oct 14, 2021 •

edited

Loading

Uh oh!

Totktonada Oct 15, 2021

Uh oh!

VitaliyaIoffe Oct 14, 2021

Uh oh!

Totktonada Oct 15, 2021

Uh oh!

VitaliyaIoffe Oct 15, 2021

Uh oh!

ligurio commented Oct 14, 2021

Uh oh!

Totktonada commented Oct 14, 2021

Uh oh!

Totktonada commented Oct 15, 2021

Uh oh!

Totktonada commented Oct 15, 2021

Uh oh!

Uh oh!

Support tags #314

Support tags #314

Uh oh!

Conversation

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

Uh oh!

Uh oh!

ligurio commented Oct 2, 2021

Uh oh!

Totktonada commented Oct 2, 2021

Uh oh!

ligurio commented Oct 4, 2021

Uh oh!

Totktonada commented Oct 5, 2021

Uh oh!

Totktonada commented Oct 5, 2021

Uh oh!

Uh oh!

VitaliyaIoffe Oct 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Totktonada Oct 15, 2021

Choose a reason for hiding this comment

Uh oh!

VitaliyaIoffe Oct 14, 2021

Choose a reason for hiding this comment

Uh oh!

Totktonada Oct 15, 2021

Choose a reason for hiding this comment

Uh oh!

VitaliyaIoffe Oct 15, 2021

Choose a reason for hiding this comment

Uh oh!

ligurio commented Oct 14, 2021

Uh oh!

Totktonada commented Oct 14, 2021

Uh oh!

Totktonada commented Oct 15, 2021

Uh oh!

Totktonada commented Oct 15, 2021

Uh oh!

Uh oh!

Totktonada commented Oct 2, 2021 •

edited

Loading

VitaliyaIoffe Oct 14, 2021 •

edited

Loading