From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp44.i.mail.ru (smtp44.i.mail.ru [94.100.177.104]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 2E0744696C3 for ; Sun, 5 Apr 2020 15:05:43 +0300 (MSK) Date: Sun, 5 Apr 2020 15:05:44 +0300 From: Alexander Turenko Message-ID: <20200405120544.sktsaxfczlkz4ehq@tkn_work_nb.domru> References: <65798b71932c21183072decd2cce8b2b0f88b884.1585773275.git.alexander.turenko@tarantool.org> <78056845-c72f-f97e-de62-2f04baaed0e0@tarantool.org> <20200403233859.57lre4ef6co22g3e@tkn_work_nb> <61a72f34-7bd0-576e-e305-dfee085ca724@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <61a72f34-7bd0-576e-e305-dfee085ca724@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH] net.box: fix fetching of schema of an old version List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org 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.