Binary Testing Framework #1485

igor-egorov · 2018-06-20T13:22:58Z

Description of the Change

The PR introduces Binary Testing Framework.
BTF allows performing stateful validation for transactions and queries built using client libraries.
For now, only Python is supported. Java support will be added later on.

This PR comes with python scripts that cover all the set of Iroha permissions.

Benefits

Now here is the tool for stateful validation of transactions and queries created via client libraries.

Also, here is a set of python examples that shows how to use Iroha permissions model. (Scripts' listings will be included into "permissions" page at ReadTheDocs).

Possible Drawbacks

No drawbacks were observed.

Usage Examples or Tests

cmake -H. -Bbuild -DSWIG_PYTHON=ON
cmake --build build --target binary_test
cd build
ctest -R binary

Possible Improvements

The existing set of python example scripts can be extended with tests which validate the correctness of resulting state after applying transactions to Iroha (that approach was not the point of current tests set).

Such improvement will require changes in the implementation of Python and Java launchers.

Signed-off-by: Igor Egorov <igor@soramitsu.co.jp>

igor-egorov · 2018-06-20T14:23:15Z

Note about SonarQube report - that is just a false positive error.

launchers.cpp#L57: The function 'initialized' is never used.

https://github.com/hyperledger/iroha/blob/53f1894ed2b8ba703f6fccb9b939b9c9a76762f7/test/integration/binary/binaries_test_fixture.hpp#L117

muratovv

I reviewed only BTF and one example. The rest of review I will provide later.

muratovv · 2018-06-21T06:32:46Z

test/integration/binary/CMakeLists.txt

+
+  add_executable(binary_test
+      launchers.cpp
+      binaries_test.cpp)


Better to move ) to the new line.

muratovv · 2018-06-21T06:33:21Z

test/integration/binary/CMakeLists.txt

+      boost
+      schema
+      shared_model_proto_backend
+      shared_model_interfaces


Looks redundant, but it is not a required issue.

muratovv · 2018-06-21T06:38:44Z

test/integration/binary/CMakeLists.txt

+  target_include_directories(binary_test PUBLIC ${PROJECT_SOURCE_DIR}/test)
+
+  add_dependencies(binary_test irohapy)
+  add_test(


Use addtest instead. https://github.com/hyperledger/iroha/blob/e8cdcc1f0b87058814e8d44c1b5f7b1839af3e92/cmake/functions.cmake#L23

Pls, note that function assumes "type" of test which is required for CI.

It is not possible, because a custom test is defined here, while addtest creates a generic test which calls the test binary without any parameters. The test call is replaced here with python interpreter call.

Can we create a generic function for those purposes? We have to manage types of tests in CI.

Maybe an easier solution would be to override add_test, do custom logic and pass the arguments to real add_test. This will add types to all tests added with add_test.

Discussed offline. Summary: there is no problem because python_binary_test target becomes visible only when SWIG for Python is enabled (-DSWIG_PYTHON=ON). Thus, CI would not be confused.

muratovv · 2018-06-21T06:39:36Z

test/integration/binary/binaries_test.cpp

+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+#include "binaries_test_fixture.hpp"


Use full path instead.

muratovv · 2018-06-21T06:40:56Z

test/integration/binary/binaries_test_fixture.hpp

+
+  namespace internal {
+
+    class Void {};


Add purpose of usage.

muratovv · 2018-06-21T06:50:05Z

test/integration/binary/launchers.hpp

+
+namespace binary_test {
+
+  class Launcher {


Add some documentation with purpose

muratovv · 2018-06-21T06:50:32Z

test/integration/binary/launchers.hpp

+   public:
+    virtual std::string launchCommand(const std::string &test_case) = 0;
+
+    void operator()(const std::string &example);


Add doc, pls

muratovv · 2018-06-21T06:52:29Z

test/integration/binary/launchers.hpp

+    std::vector<shared_model::proto::Query> queries;
+
+   protected:
+    void readBinaries(boost::process::ipstream &stream);


Add contract for the stream in a documentation

muratovv · 2018-06-21T06:55:59Z

test/integration/binary/launchers.cpp

+
+  std::string JavaLauncher::launchCommand(const std::string &example) {
+    return "";
+    // tbd, igor-egorov, 2018-06-20, IR-1389


who cares, tbd is also ok

Tbd not ok because you can't grep it fast. Better to remember only one name instead of two.

@igor-egorov you may create a snippet with TODO in your favorite IDE.

muratovv · 2018-06-21T07:00:03Z

example/python/permissions/can_add_asset_qty.py

+        .signAndAddSignature(alice['key']).finish()
+
+
+print(admin['key'].privateKey().hex())


Maybe move this framework-related code to separate class (and file) for reducing boilerplate?

Discussed with Fedor in offline.
Let's keep this code for now. We expect the protocol of communication between BTF and external scripts to be changed, so then we can reduce the amount of such boilerplate code.

l4l · 2018-06-24T14:02:58Z

example/python/permissions/can_add_asset_qty.py

+alice = commons.user('alice@test')
+
+
+def genesis_tx():


That function is pretty similar among the files, that better be moved somewhere

Finally, it was decided not to modify this place. Moving of the common part will lead to hiding command of domain creation that includes the test role. Otherwise, there will be nothing to move.

Thus, in order to increase examples' demonstrativeness, genesis functions would not be modified.

l4l · 2018-06-24T14:03:07Z

test/integration/binary/CMakeLists.txt

@@ -0,0 +1,41 @@
+
+if (SWIG_PYTHON OR SWIG_JAVA)


Consider adding a variable IS_BINDINGS or SWIG_ENABLED, etc
otherwise it be duplicated in bindings folder as well

at least mark with a todo, same for the following one

Discussed offline. Not a problem.

l4l · 2018-06-24T14:04:20Z

test/integration/binary/CMakeLists.txt

+    find_package(PythonInterp 3.5 REQUIRED)
+  endif ()
+
+  foreach (item "block" "commands" "primitive" "queries")


The dependency should be moved somewhere at separate target

l4l · 2018-06-24T14:09:22Z

test/integration/binary/binaries_test_fixture.hpp

+
+  namespace internal {
+
+    class Void {};


l4l · 2018-06-24T14:11:22Z

test/integration/binary/binaries_test_fixture.hpp

+
+}  // namespace query_validation
+
+template <typename T>


or even better rename to SdkLauncher

l4l · 2018-06-24T14:22:37Z

test/integration/binary/launchers.hpp

+
+  class PythonLauncher : public Launcher {
+   public:
+    virtual std::string launchCommand(const std::string &example);


~~virtual~~ std::string launchCommand(const std::string &example) override;
same below

l4l · 2018-06-24T14:23:52Z

test/integration/binary/launchers.hpp

+    bool initialized(const unsigned &transactions_expected = 0,
+                     const unsigned &queries_expected = 0);
+
+    std::shared_ptr<shared_model::crypto::Keypair> admin_key;


boost::optional seems more suitable (or even without any wrapper & early fail)

l4l · 2018-06-24T14:25:57Z

test/integration/binary/CMakeLists.txt

+      gtest
+      gmock
+      boost
+      schema


also redundant, it must be used in backend

l4l · 2018-06-24T14:30:17Z

example/python/permissions/commons.py

+
+
+def all_permissions():
+    return iroha.RolePermissionSet([


r = iroha.RolePermissionSet() for i in range(r.size()): r.set(i) return i

Discussed offline. Since the listing of commons.py will also be included in the documentation page about permissions, it is better to preserve fully-qualified constants' names for better presentation for documentation users.

l4l · 2018-06-24T14:31:11Z

example/python/permissions/commons.py

+    ])
+
+
+def user(user_id):


That will generate a new keypair for the same user, so consider renaming the method
or static vars hack can be used

Discussed offline. Decided to clarify method naming and not to touch the internals for now.

Signed-off-by: Igor Egorov <igor@soramitsu.co.jp>

l4l · 2018-06-30T20:03:42Z

test/integration/binary/binaries_test_fixture.hpp

+     * @param response - QueryResponse object to check
+     */
+    template <typename ExpectedResponseType>
+    inline void checkQueryResponseType(


Would be better to return bool, instead of assertion
Otherwise fails will look like Assertion failed at binaries_test_fixture.hpp:67, that is pretty useless

l4l · 2018-06-30T20:06:24Z

test/integration/binary/binaries_test_fixture.hpp

+      itf.setInitialState(launcher.admin_key.value(), genesis());
+
+      std::for_each(
+          std::next(  // first transaction was used as genesis transaction


could you move the comment at the separate line above? looks ugly :(

requested changes were applied

muratovv

Pls, fix minor issues.

muratovv · 2018-07-05T07:53:15Z

test/integration/binary/binaries_test_fixture.hpp

+#include "framework/specified_visitor.hpp"
+#include "integration/binary/launchers.hpp"
+
+using namespace boost::process;


using namespace is prohibited to use in headers.

You can use alias instead: namespace proc = boost::process

muratovv · 2018-07-05T07:58:20Z

test/integration/binary/binaries_test_fixture.hpp

+     *    execution.
+     */
+    template <typename Head, typename... Tail>
+    inline void _validateQueries(::query_validation::QueryIterator it,


What is the point of _ before the name?

Means internal, I guess

Signed-off-by: Igor Egorov <igor@soramitsu.co.jp>

The signature of AddAssetQuantity and SubtractAssetQuantity was changed. Now they take two arguments instead of three. Signed-off-by: Igor Egorov <igor@soramitsu.co.jp>

It was discovered that protobuf does not preserve type information in binary representation. That was the reason to introduce custom (BTF) type identifiers for binaries. Amount of boilerplate code in python examples was reduced. Signed-off-by: Igor Egorov <igor@soramitsu.co.jp>

sorabot · 2018-07-06T16:21:34Z

SonarQube analysis reported 1 issue

launchers.cpp#L73: The function 'initialized' is never used.

Signed-off-by: Igor Egorov <igor@soramitsu.co.jp>

Igor Egorov added 3 commits June 20, 2018 13:15

Add Binary Testing Framework with Python examples sources

a1bfb8e

Signed-off-by: Igor Egorov <igor@soramitsu.co.jp>

Apply new permissions model to python examples

0ad2b96

Signed-off-by: Igor Egorov <igor@soramitsu.co.jp>

Cosmetic improvements for BTF

53f1894

Signed-off-by: Igor Egorov <igor@soramitsu.co.jp>

igor-egorov requested review from muratovv and lebdron June 20, 2018 14:19

igor-egorov added tests pr aimed at the code coverage increase or test refactorings SDK Anything related to the client bindings labels Jun 20, 2018

muratovv previously requested changes Jun 21, 2018

View reviewed changes

l4l suggested changes Jun 24, 2018

View reviewed changes

l4l added the needs-correction pr/rfc is not completed and might be updated label Jun 26, 2018

BTF Improvements

4e7a418

Signed-off-by: Igor Egorov <igor@soramitsu.co.jp>

igor-egorov requested review from l4l and muratovv June 30, 2018 17:08

Merge remote-tracking branch 'origin/develop' into feature/btf

ab0a615

Signed-off-by: Igor Egorov <igor@soramitsu.co.jp>

l4l approved these changes Jun 30, 2018

View reviewed changes

muratovv approved these changes Jul 5, 2018

View reviewed changes

l4l removed the needs-correction pr/rfc is not completed and might be updated label Jul 5, 2018

Igor Egorov added 5 commits July 5, 2018 20:14

Fix review comments, BTF

d61d83c

Signed-off-by: Igor Egorov <igor@soramitsu.co.jp>

Merge remote-tracking branch 'origin/develop' into feature/btf

76f9c94

Fix formatting

ed76d13

Signed-off-by: Igor Egorov <igor@soramitsu.co.jp>

Update add/subtract asset quantity examples

aed49f6

The signature of AddAssetQuantity and SubtractAssetQuantity was changed. Now they take two arguments instead of three. Signed-off-by: Igor Egorov <igor@soramitsu.co.jp>

lebdron approved these changes Jul 6, 2018

View reviewed changes

Merge remote-tracking branch 'origin/develop' into feature/btf

ec2b9f4

igor-egorov merged commit 4c71d49 into develop Jul 6, 2018

igor-egorov deleted the feature/btf branch July 6, 2018 17:35

neewy mentioned this pull request Jul 25, 2018

Hyperledger Iroha v1.0 beta-4 version #1597

Merged

l4l pushed a commit that referenced this pull request Jul 25, 2018

Binary Testing Framework (#1485)

7739bd7

Signed-off-by: Igor Egorov <igor@soramitsu.co.jp>

l4l pushed a commit that referenced this pull request Jul 25, 2018

Binary Testing Framework (#1485)

71de667

Signed-off-by: Igor Egorov <igor@soramitsu.co.jp>

l4l pushed a commit that referenced this pull request Jul 25, 2018

Binary Testing Framework (#1485)

7dfd6cc

Signed-off-by: Igor Egorov <igor@soramitsu.co.jp>

l4l pushed a commit that referenced this pull request Jul 25, 2018

Binary Testing Framework (#1485)

36c5ebc

Signed-off-by: Igor Egorov <igor@soramitsu.co.jp>

l4l pushed a commit that referenced this pull request Jul 25, 2018

Binary Testing Framework (#1485)

df5062f

Signed-off-by: Igor Egorov <igor@soramitsu.co.jp>

		.signAndAddSignature(alice['key']).finish()


		print(admin['key'].privateKey().hex())

		alice = commons.user('alice@test')


		def genesis_tx():

		])


		def user(user_id):

Binary Testing Framework #1485

Binary Testing Framework #1485

Conversation

igor-egorov commented Jun 20, 2018

Description of the Change

Benefits

Possible Drawbacks

Usage Examples or Tests

Possible Improvements

igor-egorov commented Jun 20, 2018

muratovv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lebdron Jun 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muratovv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorabot commented Jul 6, 2018

lebdron Jun 21, 2018 •

edited

Loading