[Tarantool-patches] [PATCH] net.box: fix fetching of schema of an old version

[Alexander Turenko]

On Sat, Apr 04, 2020 at 07:40:44PM +0200, Vladislav Shpilevoy wrote:
> >>> https://github.com/tarantool/tarantool/issues/4691
> >>> https://github.com/tarantool/tarantool/tree/Totktonada/gh-4691-net-box-connect-schema-2-1-3
> >>>
> >>>  src/box/lua/net_box.lua                       |   9 ++
> >>>  ...4691-net-box-connect-schema-2-1-3.test.lua |  93 ++++++++++++++++++
> >>>  test/box-tap/no_auto_schema_upgrade.lua       |  30 ++++++
> >>>  .../snap/2.1.3/00000000000000000000.snap      | Bin 0 -> 4466 bytes
> >>>  4 files changed, 132 insertions(+)
> >>>  create mode 100755 test/box-tap/gh-4691-net-box-connect-schema-2-1-3.test.lua
> >>
> >> We have xlog/upgrade suite specifically for these tests. With
> >> already designed processes, where and how to store old snaps, etc.
> >> See xlog/upgrade/how_to_add_new_test.md.
> > 
> > 
> > I want to test the case when an instance is NOT upgraded. So 'upgrade'
> > directory name for snaps would be misleading.
> 
> The test and the bug are still upgrade related. 'xlog/upgrade' is not
> called 'xlog/upgrade_called'. It is called just 'upgrade'. For all cases
> when an old snapshot is used to start a new tarantool.

I disagree here. 'upgrade' is not something about working upward an old
snapshot.

> 
> > I also think that we should move from using compare-against-output
> > tests.
> 
> This is not about tap vs diff. It is about having all upgrade related
> tests in one place, and organized in one way, with how to fill them, with
> how to store xlogs and snapshots in the repository.

But I still want to test it using TAP13 test. test/xlog is the test
suite that is fed to the console.

> 
> > I'll not insist on this during review, but personally I will
> > create new tests using pytest-like (phpunit-like, maven-failsafe-like,
> > luatest-like) way (it is our TAP13 tests or at least near to them). I
> > can summarize cons and pros if you want (don't sure whether I did it
> > already somewhere).
> 
> Don't really want to discuss this again, everything was said already
> 1000 times or more.

I'm too.

> 
> > We can move 'snap' directory upward (right into test/) and use it for
> > snapshots for all test suites. Or keep them per-suite. However see the
> > next paragraph re fill.lua.
> 
> This is a bad idea, because different upgrade-related tests usually
> need some specific data stored in the old snapshot. If you merge data,
> needed by many tests, into one snapshot, you will violate the policy of
> one-bug-one-test. Because many tests will start to depend on each other.

Ouch, okay, fill.lua is called on the old tarantool version (on which
you generate a snapshot). It is okay for my case to disable autoupgrade
and insert the data using the new tarantool version. I guess it is okay
too for most of cases. Your case with upgrade of a sequence can be
tested this way too.

Inserting necessary data right from a test seems to be more sane
approach: you don't need to jump over several files to look at the test
case at whole: its data and code.

This way also splits snapshot from test data. The snapshot contains just
what appears after box.cfg() + box.snapshot() on an old tarantool
version. No surprises.

> 
> > I prefer to feed initial schema and data right from a test to keep them
> > near to actual usage in test cases (within one file).
> 
> This makes impossible to test bugs, which appear only when you bootstrap
> from the old snapshot.

Any example? Your case with a sequence upgrade can be tested with
inserting data using the new tarantool version.

We just need to start tarantool of the new version on the old snapshot
in the read-only mode to disable autoupgrading, enable read-write,
insert necessary data. Then we can either test the instance on the old
schema or call upgrade and test it after upgrade.

> 
> > Sometimes it eases
> > reading of test cases, especially when you're not much familiar with a
> > testing system.
> 
> You are now creating a second way of testing upgrade. This is definitely
> not a simplification of the testing system.

Nope. Again, I don't test upgrade here.

But, yep, I want to enforce several properties and so I'm unable to use
the way you proposed to run tarantool on an old schema:

* A test is the script with TAP13 output, so it should be inside
  a corresponding test suite.
* A test inserts all needed data right from the test file. The instance
  file just calls box.cfg() and give guest user accesses. It is highly
  reusable so.

> 
> > I don't much like the approach with fill.lua script (at
> > least for data corresponds to test cases).
> 
> fill.lua is inevitable, when you need to test bootstrap from an old
> snapshot having some data in it before you start tarantool, and you want
> to know what data is stored here exactly.

I would call for examples.

But anyway, we can keep both ways and use fill.lua when it is not
possible to feed initial data for a test using the new tarantool
version.