ITF checking statuses set #1720

l4l · 2018-09-15T08:52:34Z

Description of the Change

ITF before only able to check, whether the sent tx is stateless valid or not (only the first status). This RP introduce the ITF feature to check against all the specified statuses, both for single transaction and tx sequence.

Benefits

It is possible to check all statuses with one itf command, their order and responses content (e.g error message)

Possible Drawbacks

~~Current implementation doesn't check the order of passed transaction. Is it an issue?~~

Usage Examples or Tests

As a usage example CreateRole::Basic acceptance test was reworked according to new ITF::sendTx.
Also for ITF::sendTxSequence there was reworked a fn call in BatchPipelineTest::InvalidAtomicBatch

Signed-off-by: Kitsu <mail@kitsu.me>

l4l · 2018-09-15T08:53:29Z

test/framework/integration_framework/integration_test_framework.cpp

+        .filter([&](auto s) { return s->transactionHash() == tx.hash(); })
+        .map([](auto s) { return TxStatus(s->get().which()); })
+        .filter([&statuses](auto s) { return statuses.count(s) != 0; })
+        .take_while([&statuses](auto) { return statuses.size() != 0; })


This chain calls probably can be improved, feel free to propose solution

Signed-off-by: Kitsu <mail@kitsu.me>

kamilsa · 2018-09-17T10:26:26Z

test/framework/integration_framework/integration_test_framework.hpp

+    /**
+     * Send transactions to Iroha and validate obtained statuses
+     * @param tx_sequence - transactions sequence
+     * @param statuses - vector of statues list to be checked against, which


typo with statues

kamilsa · 2018-09-17T10:39:55Z

test/framework/integration_framework/integration_test_framework.cpp

+    iroha_instance_->getIrohaInstance()->getCommandService()->ListTorii(
+        tx_list);
+
+    // wait, until all of the statuses are recieved


typo: received

kamilsa · 2018-09-17T11:38:26Z

test/framework/integration_framework/integration_test_framework.cpp

+    iroha_instance_->instance_->getStatusBus()
+        ->statuses()
+        .filter([&](auto s) { return s->transactionHash() == tx.hash(); })
+        .map([](auto s) { return TxStatus(s->get().which()); })


dangerous cast from variant to enum. If someone changes the order of types in either variant or enum it will be hard to find the reason of the error. Maybe cast it explicitly via visit_in_place?

kamilsa · 2018-09-17T11:44:35Z

test/framework/integration_framework/integration_test_framework.cpp

+          return tx_statsues.find(s->transactionHash()) != tx_statsues.end();
+        })
+        .subscribe([&](auto s) {
+          auto status = TxStatus(s->get().which());


same. Maybe we can define a separate function, responsible for such conversion?

Signed-off-by: Kitsu <mail@kitsu.me>

lebdron

This approach does not allow to compare the error message in responses, and introduces code duplication for statuses. Consider accepting a visitor for each transaction.

kamilsa · 2018-09-18T10:20:12Z

test/framework/integration_framework/integration_test_framework.cpp

+
+    // mapping transaction hash to its statuses that need to be checked
+    std::map<shared_model::crypto::Hash, std::set<TxStatus>, HashCmp>
+        tx_statsues;


Fix that typo

This reverts commit 047cb29. Signed-off-by: Kitsu <mail@kitsu.me>

Signed-off-by: Kitsu <mail@kitsu.me>

kamilsa · 2018-09-24T09:30:02Z

test/framework/integration_framework/integration_test_framework.cpp

-          if (response != statuses.end()) {
-            statuses.erase(response);
-          }
+          status_list.emplace_back(iroha::protocol::ToriiResponse(


Can we make status list to be vector of interfaces and use clone function instead of copying protobuf explicitly?

kamilsa · 2018-09-24T09:30:18Z

test/framework/integration_framework/integration_test_framework.cpp

-      const shared_model::proto::Transaction &tx, std::set<TxStatus> statuses) {
+      const shared_model::proto::Transaction &tx,
+      std::function<
+          void(const std::vector<shared_model::proto::TransactionResponse> &)>


Can we change to interface::TransactionResponse?

only if it std::vector<std::shared_ptr<iface::TxResponse>>

kamilsa · 2018-09-24T09:31:09Z

test/framework/integration_framework/integration_test_framework.hpp

@@ -153,8 +153,12 @@ namespace integration_framework {
     * @param statuses - list of statuses to check against
     * @return this
     */


Update comment

kamilsa · 2018-09-24T09:31:32Z

test/integration/acceptance/create_role_test.cpp

+          makeUserWithPerms(),
+          [](auto &resps) {
+            for (auto &&r : resps) {
+              std::cout << r.get().which() << std::endl;


Did yoy forget to remove that?

kamilsa · 2018-09-24T09:32:16Z

test/framework/integration_framework/integration_test_framework.hpp

-    IntegrationTestFramework &sendTx(const shared_model::proto::Transaction &tx,
-                                     std::set<TxStatus> statuses);
+    IntegrationTestFramework &sendTx(
+        const shared_model::proto::Transaction &tx,


Also, can we work with interfaces here?

it's consistent with ITF interface, so what's the point?

Signed-off-by: Kitsu <mail@kitsu.me>

sorabot · 2018-09-25T21:48:23Z

SonarQube analysis reported 3 issues

integration_test_framework.cpp#L319: Reference to auto variable returned.
acceptance_fixture.hpp#L162: The class 'PipelineIntegrationTest' defines member variable with name 'kAdminId' also defined in its parent class 'AcceptanceFixture'.
acceptance_fixture.hpp#L163: The class 'PipelineIntegrationTest' defines member variable with name 'kAdminKeypair' also defined in its parent class 'AcceptanceFixture'.

lebdron

status_count approach will not work because in case a new status is added, it may be required to refactor all the tests with the new value. It is required to discuss another possible approaches.

Signed-off-by: Kitsu <mail@kitsu.me>

lebdron · 2018-10-04T08:45:51Z

test/framework/integration_framework/integration_test_framework.hpp

@@ -276,6 +289,18 @@ namespace integration_framework {
    tbb::concurrent_queue<ProposalType> proposal_queue_;
    tbb::concurrent_queue<ProposalType> verified_proposal_queue_;
    tbb::concurrent_queue<BlockType> block_queue_;
+
+    struct HashCmp {


Is this really required? Maybe use std::string from hash hex instead?

lebdron · 2018-10-04T08:47:10Z

test/framework/integration_framework/integration_test_framework.hpp

+    };
+
+    std::map<shared_model::interface::types::HashType,
+             tbb::concurrent_queue<TxResponseType>,


Observable does not emit values concurrently, so there is no need in a concurrent queue for pushes. Reads are also done in main test thread, so read concurrency is also not required.

gRPC-based thread is writing here and the ITF main thread is reading. So there should be concurrent queue

lebdron · 2018-10-04T08:51:16Z

test/integration/acceptance/acceptance_fixture.cpp

+  template <typename T>
+  void checkStatus(const shared_model::interface::TransactionResponse &resp) {
+    ASSERT_NO_THROW(
+        boost::apply_visitor(framework::SpecifiedVisitor<T>(), resp.get()));


Please use boost::get instead.

lebdron · 2018-10-04T08:51:38Z

test/integration/acceptance/acceptance_fixture.cpp

@@ -25,11 +34,24 @@ AcceptanceFixture::AcceptanceFixture()
          shared_model::crypto::DefaultCryptoAlgorithmType::generateKeypair()),
      kUserKeypair(
          shared_model::crypto::DefaultCryptoAlgorithmType::generateKeypair()),
+      checkEnoughSignatures([](auto &status) {
+        checkStatus<shared_model::interface::EnoughSignaturesCollectedResponse>(


This looks copy-pasted. Define and instantiate a template variable instead:

template <typename T> const std::function<void(const shared_model::proto::TransactionResponse &)> check_status = [](const shared_model::proto::TransactionResponse &) { ... } template const std::function<void(const shared_model::proto::TransactionResponse &)> check_status<shared_model::interface::EnoughSignaturesCollectedResponse>;

These are just comfy aliases to checkStatus itself.
It's much more descriptive:
itf.checkStatus(checkEnoughSignatures)

rather than:
itf.checkStatus(check_status<shared_model::interface::EnoughSignaturesCollectedResponse)

OK, but is it possible to initialize the variables with the function directly, so that it is not required to wrap checkStatus<T> in a lambda? This will reduce code in this file at least.

lebdron · 2018-10-04T08:59:58Z

test/integration/acceptance/acceptance_fixture.hpp

@@ -162,8 +163,18 @@ class AcceptanceFixture : public ::testing::Test {
  const shared_model::crypto::Keypair kAdminKeypair;
  const shared_model::crypto::Keypair kUserKeypair;

+  const std::function<void(const shared_model::proto::TransactionResponse &)>


Looks like copy-paste. Declare a template variable:

template <typename T> const std::function<void(const shared_model::proto::TransactionResponse &)> check_status;

You can declare the variables with just once type to avoid duplication:
const std::function<void(const TxResponseType &)> checkEnoughSignatures, checkStatelessInvalid, ...

lebdron · 2018-10-04T09:06:59Z

test/integration/acceptance/acceptance_fixture.cpp

+namespace {
+  template <typename T>
+  void checkStatus(const shared_model::interface::TransactionResponse &resp) {
+    ASSERT_NO_THROW(


Assertions are better done in the test body, so that log contains a reference to line of test code.

lebdron · 2018-10-04T09:08:10Z

test/integration/acceptance/acceptance_fixture.hpp

@@ -11,6 +11,7 @@
 #include <string>
 #include <vector>
 #include "cryptography/keypair.hpp"
+#include "framework/integration_framework/integration_test_framework.hpp"


It is unused, so it can be removed.

Signed-off-by: Kitsu <mail@kitsu.me>

lebdron · 2018-10-10T06:39:16Z

test/framework/integration_framework/integration_test_framework.cpp

@@ -163,6 +165,12 @@ namespace integration_framework {
          log_->info("commit");
          queue_cond.notify_all();
        });
+    iroha_instance_->getIrohaInstance()->getStatusBus()->statuses().subscribe(
+        [this](auto responses) {


responses -> response

lebdron · 2018-10-10T06:40:31Z

test/framework/integration_framework/integration_test_framework.cpp

+      const shared_model::interface::types::HashType &tx_hash,
+      std::function<void(const shared_model::proto::TransactionResponse &)>
+          validation) {
+    // fetch first proposal from proposal queue


This comment looks out of place, please rephrase appropriately.

lebdron · 2018-10-10T06:49:28Z

test/framework/integration_framework/integration_test_framework.cpp

@@ -375,6 +383,21 @@ namespace integration_framework {
    return *this;
  }

+  IntegrationTestFramework &IntegrationTestFramework::checkStatus(
+      const shared_model::interface::types::HashType &tx_hash,
+      std::function<void(const shared_model::proto::TransactionResponse &)>


checkProposal, checkVerifiedProposal, checkBlock use interface type for lambdas. Please use it here as well for consistency:
std::function<void(const TxResponseType &)>

lebdron · 2018-10-10T06:50:50Z

test/integration/acceptance/acceptance_fixture.cpp

@@ -25,11 +34,24 @@ AcceptanceFixture::AcceptanceFixture()
          shared_model::crypto::DefaultCryptoAlgorithmType::generateKeypair()),
      kUserKeypair(
          shared_model::crypto::DefaultCryptoAlgorithmType::generateKeypair()),
+      checkEnoughSignatures([](auto &status) {
+        checkStatus<shared_model::interface::EnoughSignaturesCollectedResponse>(


OK, but is it possible to initialize the variables with the function directly, so that it is not required to wrap checkStatus<T> in a lambda? This will reduce code in this file at least.

lebdron · 2018-10-10T06:52:25Z

test/integration/acceptance/acceptance_fixture.hpp

@@ -162,8 +163,18 @@ class AcceptanceFixture : public ::testing::Test {
  const shared_model::crypto::Keypair kAdminKeypair;
  const shared_model::crypto::Keypair kUserKeypair;

+  const std::function<void(const shared_model::proto::TransactionResponse &)>


You can declare the variables with just once type to avoid duplication:
const std::function<void(const TxResponseType &)> checkEnoughSignatures, checkStatelessInvalid, ...

Signed-off-by: Kitsu <mail@kitsu.me>

kamilsa · 2018-10-16T07:44:53Z

test/integration/acceptance/acceptance_fixture.hpp

+
+#define CHECK_COMMITTED                                                  \
+  [](const shared_model::interface::TransactionResponse &resp) {         \
+    SCOPED_TRACE("CHECK_COMMITTE");                                      \


find&replace ninja failed :( thanks!

Signed-off-by: Kitsu <mail@kitsu.me>

lebdron · 2018-10-18T07:49:13Z

test/integration/acceptance/acceptance_fixture.hpp

-}  // namespace shared_model
+namespace {
+  template <typename Type>
+  void __checkFunction(


Codestyle requires camelcase names without underscores for functions. Leading underscores are used by system and compiler methods, so there is no need for them here.

lebdron · 2018-10-18T08:03:56Z

test/integration/acceptance/acceptance_fixture.hpp

+  }
+
+#define CHECK_ENOUGH_SIGNATURES                                            \
+  [](const shared_model::interface::TransactionResponse &resp) {           \


Please separate these macros into two:

#define VALIDATE_RESPONSE(type) \ [](const shared_model::interface::TransactionResponse &resp) { \ SCOPED_TRACE("#type"); \ checkFunction<shared_model::interface::##type>(resp); \ } #define CHECK_ENOUGH_SIGNATURES VALIDATE_RESPONSE(EnoughSignaturesCollectedResponse)

User proper naming for internal checking repsonse function. Also reuse code in macros. Signed-off-by: Kitsu <mail@kitsu.me>

Signed-off-by: Kitsu <mail@kitsu.me>

Add support ITF::sendTx for checking all statuses

047cb29

Signed-off-by: Kitsu <mail@kitsu.me>

l4l added needs-review pr awaits review from maintainers tests pr aimed at the code coverage increase or test refactorings labels Sep 15, 2018

l4l requested review from kamilsa and lebdron September 15, 2018 08:52

l4l commented Sep 15, 2018

View reviewed changes

l4l added 2 commits September 15, 2018 13:54

Add support ITF::sendTxSeq for checking all statuses

69342c6

Signed-off-by: Kitsu <mail@kitsu.me>

Add comments to sendTx/sendTxSeq with statuses

90ba876

Signed-off-by: Kitsu <mail@kitsu.me>

kamilsa suggested changes Sep 17, 2018

View reviewed changes

l4l added 3 commits September 18, 2018 00:14

Fix typos

5f77aaa

Signed-off-by: Kitsu <mail@kitsu.me>

Use compile-time checks for tx status convertion

bc670bf

Signed-off-by: Kitsu <mail@kitsu.me>

Add missing const qualifier for HashCmp

7ea0b22

Signed-off-by: Kitsu <mail@kitsu.me>

kamilsa approved these changes Sep 18, 2018

View reviewed changes

lebdron suggested changes Sep 18, 2018

View reviewed changes

kamilsa reviewed Sep 18, 2018

View reviewed changes

l4l added 2 commits September 23, 2018 18:13

Revert "Add support ITF::sendTx for checking all statuses"

e5a5d8a

This reverts commit 047cb29. Signed-off-by: Kitsu <mail@kitsu.me>

Check at once all statuses in ITF::sendTx

aa000a8

Signed-off-by: Kitsu <mail@kitsu.me>

l4l force-pushed the feature/itf_statuses branch from fa2421a to aa000a8 Compare September 23, 2018 15:24

kamilsa suggested changes Sep 24, 2018

View reviewed changes

Rework sendTxSequence

a27006d

Signed-off-by: Kitsu <mail@kitsu.me>

lebdron suggested changes Sep 26, 2018

View reviewed changes

Merge branch 'dev' into feature/itf_statuses

6c3f151

Signed-off-by: Kitsu <mail@kitsu.me>

l4l force-pushed the feature/itf_statuses branch from 537b522 to 6c3f151 Compare October 2, 2018 22:57

Rework tx status get via ITF::checkStatus

56bbbe5

Signed-off-by: Kitsu <mail@kitsu.me>

lebdron suggested changes Oct 4, 2018

View reviewed changes

l4l added 2 commits October 4, 2018 15:55

Cleanup framework files

694a51e

Signed-off-by: Kitsu <mail@kitsu.me>

Fix AF::checkStatus

0eb9c3b

Signed-off-by: Kitsu <mail@kitsu.me>

lebdron suggested changes Oct 10, 2018

View reviewed changes

Rework checkers with macro

649207a

Signed-off-by: Kitsu <mail@kitsu.me>

Add review fixes

9032958

Signed-off-by: Kitsu <mail@kitsu.me>

kamilsa reviewed Oct 16, 2018

View reviewed changes

l4l added 2 commits October 16, 2018 14:27

Merge branch 'dev' into feature/itf_statuses

cca8fa3

Signed-off-by: Kitsu <mail@kitsu.me>

Fix CHECK_COMITTED typo

1b5c92b

Signed-off-by: Kitsu <mail@kitsu.me>

kamilsa approved these changes Oct 17, 2018

View reviewed changes

lebdron approved these changes Oct 18, 2018

View reviewed changes

Add review fixes

f873d2e

User proper naming for internal checking repsonse function. Also reuse code in macros. Signed-off-by: Kitsu <mail@kitsu.me>

l4l force-pushed the feature/itf_statuses branch from ef112a9 to f873d2e Compare October 18, 2018 10:20

l4l merged commit 001fac8 into dev Oct 18, 2018

l4l deleted the feature/itf_statuses branch October 18, 2018 11:36

kamilsa mentioned this pull request Nov 1, 2018

Hyperledger Iroha v1.0 beta-5 version #1825

Closed

bakhtin pushed a commit that referenced this pull request Nov 2, 2018

ITF checking statuses set (#1720)

371e3c8

Signed-off-by: Kitsu <mail@kitsu.me>

neewy mentioned this pull request Nov 2, 2018

Hyperledger Iroha v1.0 beta-5 version #1832

Closed

kamilsa mentioned this pull request Dec 12, 2018

Hyperledger Iroha v1.0 Release Candidate 1 #1941

Merged

ITF checking statuses set #1720

ITF checking statuses set #1720

Conversation

l4l commented Sep 15, 2018 • edited Loading

Description of the Change

Benefits

Possible Drawbacks

Usage Examples or Tests

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

lebdron 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

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 Sep 25, 2018

lebdron 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

Choose a reason for hiding this comment

l4l Oct 4, 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

l4l commented Sep 15, 2018 •

edited

Loading

l4l Oct 4, 2018 •

edited

Loading