Update and refine the readme #323

NickVolynkin · 2022-02-14T14:41:53Z

Preview: https://github.com/NickVolynkin/test-run/blob/update-readme/README.md

coveralls · 2022-02-14T15:19:19Z

Coverage increased (+0.04%) to 63.598% when pulling ef57de9 on NickVolynkin:update-readme into f246567 on tarantool:master.

README.md

Totktonada · 2022-02-18T00:20:54Z

README.md

+### Other parameters
+
+
+* `show_reproduce_content` (optional, `True` by default) — TODO: what's this?


thanks, I've added this info to the document

However now we just re-run the server each time. It seems, the option has no sense (as well as reproduce files at all).

OTOH, there are at least particular cases, where it increases run time more than twice, see the table in tarantool/tarantool#6849 (comment). Are there chances that we'll return to running of several tests within one tarantool instance? Don't know. I would better pay some machine resources to restart tarantools rather than human resources to investigate tricky problems.

I've found a single use case: ./sql-tap/suite.ini:31:show_reproduce_content = False. So, for all other tests this value is True (by default), which is useless — is that really so?

Totktonada · 2022-02-18T00:22:20Z

README.md

+* `show_reproduce_content` (optional, `True` by default) — TODO: what's this?
+* `is_parallel` (optional, `False` by default) — whether the tests in the suite can run in parallel
+
+* `use_unix_sockets` (optional, `False` by default) — use hard-coded UNIX sockets


For console, AFAIR.

'Hard-coded' is a bit too hard words. It depends of server's script name.

Totktonada · 2022-02-18T00:26:48Z

README.md

+Next, the test in `.test(.lua|.py)?` is executed. 
+If this is a first test run and there's no `.result` file yet, it is generated from the output.
+If there is a `.result` file, output is written to a `.reject` file,
+which is then compared to `.result`.


.test.sql as well.

And _test.lua in luatest test suites.

The autowritting .result file was removed together with introducing the --update-result option.

There is no a word about TAP13 compatible output (it does not need a result file).

I've removed enumeration of test file extensions from this line, it resolves (1) and (2). Also, I won't mention writing .result files at all. Moved it to a separate issue: #327

Added a few words about TAP13

Totktonada · 2022-02-18T00:27:20Z

README.md

+If there's a difference between `.reject` and `.result`, the following happens:
+
+1. The last 15 lines of diff are printed to output.
+2. The `.reject` file is saved in the `<vardir>/rejects/<suite>`
+   subfolder given in options or set locally as `var/rejects/<suite>` by default.
+
+If there is no difference between `.reject` and `.result`, the `.reject` file is deleted.


Don't know, whether it is useful, to be honest.

Totktonada · 2022-02-18T18:42:40Z

README.md

+
+* `description`
+
+* `script` — A file with Tarantool commands.


I would call a core = tarantool test as 'a file with tarantool commands'. Instance file is a Lua script.

Totktonada · 2022-02-18T18:45:04Z

README.md

+Each test consists of the following files:
+
+* Test file: `<name>.test.lua`, `<name>_test.lua`, `<name>.test.py`, or `<name>.test.sql`.
+* Reference file: `<name>.result`.
+* Skip condition file: `<name>.skipcond`.


The altter two bullets are optional, but the text looks like they're mandatory.

Totktonada · 2022-02-18T18:51:04Z

README.md

+The optional skip condition file is a Python script.
+In the local Python environment of a test run there's a `self` object,
+which is an instance of the [`Test` class](./lib/test.py).
+Set `self.skip = 1` to skip this test.


I would leave a couple of words about the idea of the skipcond file. The typical usage is to skip a test on given OS, say, on Mac OS.

Totktonada · 2022-02-18T19:09:19Z

README.md


-Files: `<name>.test.py`, `<name>.result` and `<name>.skipcond`(optionaly).
+Files: `<name>.test.py`, `<name>.result` and `<name>.skipcond` (optionally).

 Environment:


sql was changed to remote in 2015 (a9adaf5) and then renamed to iproto (6adba83). Also the BoxConnection class definitely changes its meaning since those times. The examples below are irrelevant.

While admin below still actual.

I also suggest to re-verify that the example below still work.

I don't want to touch this part of readme in this PR. Will review it later.

How I'll differentiate parts, where problems are left deliberately and where it is unintentional? No, I think your point does not work here: your changes are not so dense.

Totktonada · 2022-02-18T19:19:38Z

README.md

@@ -166,14 +302,14 @@ box.info.lsn
 ...
 ```

-#### Lua
+### Lua

 Files: `<name>.test.lua`, `<name>.result` and `<name>.skipcond`(optionaly).
 Tests interact only with `AdminConnection`. Supports some preprocessor functions (eg `delimiter`)


Tests interact only with AdminConnection.

Don't know what it means, but it looks irrelevant now.

Totktonada · 2022-02-18T19:20:27Z

README.md

@@ -166,14 +302,14 @@ box.info.lsn
 ...
 ```

-#### Lua
+### Lua


core = tarantool and core = app tests are really different. It worth to split its description.

Totktonada · 2022-02-18T19:22:57Z

README.md

@@ -337,7 +473,7 @@ test_run:cmd('setopt delimiter ""');
 join(test_run, 30)
 ```

-### pretest_clean()
+## pretest_clean()


That's strange that a function name is here. Let's give the section a human readable title.

Totktonada · 2022-02-18T19:30:07Z

That's really boring to write all those details. Maybe it is better to book 1,5 hours and traverse over all features and tricks of test-run known to me. We can start with reviewing the readme file and expend it beyond if time will permit. Or, maybe, a hour about user visible behaviour and a hour for internal details.

Totktonada · 2022-03-04T14:11:18Z

My comments are mostly not answered. It is in the responsibility of a developer to update all discussions before asking for a next review. Otherwise a reviewer will be obligated to re-check every point, which (s)he highlighted. It is time consuming and not profitable.

I see a temporary commit in this pull request. I expect ready-to-go (in your opinion) pull request, when a review is acquired. Please, elaborate.

After this I'll look at your answers and, if nothing will confuse me, can skip re-check of the changes. You'll wait less time for review so.

Please, take me right. I review 50-60 PRs per month. I would not able to do so without 'extra' effort from developers to make reviews simpler.

Totktonada · 2022-03-16T23:51:39Z

My comments are mostly not answered.

Still. Please, find another reviewer.

NickVolynkin force-pushed the update-readme branch from 46d6813 to 75e528d Compare February 14, 2022 14:47

NickVolynkin force-pushed the update-readme branch 6 times, most recently from 1a1002f to 258273e Compare February 15, 2022 14:17

NickVolynkin requested a review from Totktonada February 15, 2022 14:28

Totktonada reviewed Feb 17, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

Totktonada reviewed Feb 17, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved