Fix pure virtual method call with fakeit

arangodb · mchacki · Nov 18, 2019 · Oct 30, 2019 · Oct 30, 2019 · Oct 30, 2019
commit fb37db0c4a4ece6ba46b6cbd6242cf519b0767b3
diff --git a/arangod/Aql/ExecutionBlock.cpp b/arangod/Aql/ExecutionBlock.cpp
@@ -70,7 +70,7 @@ std::string const& stateToString(aql::ExecutionState state) {
 
 ExecutionBlock::ExecutionBlock(ExecutionEngine* engine, ExecutionNode const* ep)
     : _engine(engine),
-      _trx(engine->getQuery()->trx()),
+      _trxVpackOptions(engine->getQuery()->trx()->transactionContextPtr()->getVPackOptions()),
       _shutdownResult(TRI_ERROR_NO_ERROR),
       _done(false),
       _isInSplicedSubquery(ep != nullptr ? ep->isInSplicedSubquery() : false),
@@ -190,7 +190,7 @@ std::pair<ExecutionState, SharedAqlItemBlockPtr> ExecutionBlock::traceGetSomeEnd
               << "getSome type=" << node->getTypeString() << " result: nullptr";
         } else {
           VPackBuilder builder;
-          auto options = transaction()->transactionContextPtr()->getVPackOptions();
+          auto const options = trxVpackOptions();
           result->toSimpleVPack(options, builder);
           LOG_TOPIC("fcd9c", INFO, Logger::QUERIES)
               << "[query#" << queryId << "] "
@@ -272,7 +272,9 @@ ExecutionState ExecutionBlock::getHasMoreState() {
 
 ExecutionNode const* ExecutionBlock::getPlanNode() const { return _exeNode; }
 
-transaction::Methods* ExecutionBlock::transaction() const { return _trx; }
+velocypack::Options const* ExecutionBlock::trxVpackOptions() const noexcept {
+  return _trxVpackOptions;
+}
 
 void ExecutionBlock::addDependency(ExecutionBlock* ep) {
   TRI_ASSERT(ep != nullptr);

diff --git a/arangod/Aql/ExecutionBlock.h b/arangod/Aql/ExecutionBlock.h
@@ -55,15 +55,15 @@ class ExecutionBlock {
 
  public:
   /// @brief batch size value
-  static constexpr inline size_t DefaultBatchSize() { return 1000; }
+  [[nodiscard]] static constexpr inline size_t DefaultBatchSize() { return 1000; }
 
   /// @brief Number to use when we skip all. Should really be inf, but don't
   /// use something near std::numeric_limits<size_t>::max() to avoid overflows
   /// in calculations.
   /// This is used as an argument for skipSome(), e.g. when counting everything.
   /// Setting this to any other value >0 does not (and must not) affect the
   /// results. It's only to reduce the number of necessary skipSome calls.
-  static constexpr inline size_t SkipAllSize() { return 1000000000; }
+  [[nodiscard]] static constexpr inline size_t SkipAllSize() { return 1000000000; }
 
   /// @brief Methods for execution
   /// Lifecycle is:
@@ -78,10 +78,10 @@ class ExecutionBlock {
   ///    DESTRUCTOR
 
   /// @brief initializeCursor, could be called multiple times
-  virtual std::pair<ExecutionState, Result> initializeCursor(InputAqlItemRow const& input);
+  [[nodiscard]] virtual std::pair<ExecutionState, Result> initializeCursor(InputAqlItemRow const& input);
 
   /// @brief shutdown, will be called exactly once for the whole query
-  virtual std::pair<ExecutionState, Result> shutdown(int errorCode);
+  [[nodiscard]] virtual std::pair<ExecutionState, Result> shutdown(int errorCode);
 
   /// @brief getSome, gets some more items, semantic is as follows: not
   /// more than atMost items may be delivered. The method tries to
@@ -90,46 +90,45 @@ class ExecutionBlock {
   /// if it returns an actual block, it must contain at least one item.
   /// getSome() also takes care of tracing and clearing registers; don't do it
   /// in getOrSkipSome() implementations.
-  virtual std::pair<ExecutionState, SharedAqlItemBlockPtr> getSome(size_t atMost) = 0;
+  [[nodiscard]] virtual std::pair<ExecutionState, SharedAqlItemBlockPtr> getSome(size_t atMost) = 0;
 
   // Trace the start of a getSome call
   void traceGetSomeBegin(size_t atMost);
 
   // Trace the end of a getSome call, potentially with result
-  std::pair<ExecutionState, SharedAqlItemBlockPtr> traceGetSomeEnd(
+  [[nodiscard]] std::pair<ExecutionState, SharedAqlItemBlockPtr> traceGetSomeEnd(
       ExecutionState state, SharedAqlItemBlockPtr result);
 
   void traceSkipSomeBegin(size_t atMost);
 
-  std::pair<ExecutionState, size_t> traceSkipSomeEnd(std::pair<ExecutionState, size_t> res);
+  [[nodiscard]] std::pair<ExecutionState, size_t> traceSkipSomeEnd(std::pair<ExecutionState, size_t> res);
 
-  std::pair<ExecutionState, size_t> traceSkipSomeEnd(ExecutionState state, size_t skipped);
+  [[nodiscard]] std::pair<ExecutionState, size_t> traceSkipSomeEnd(ExecutionState state, size_t skipped);
 
   /// @brief skipSome, skips some more items, semantic is as follows: not
   /// more than atMost items may be skipped. The method tries to
   /// skip a block of at most atMost items, however, it may skip
   /// less (for example if there are not enough items to come). The number of
   /// elements skipped is returned.
-  virtual std::pair<ExecutionState, size_t> skipSome(size_t atMost) = 0;
+  [[nodiscard]] virtual std::pair<ExecutionState, size_t> skipSome(size_t atMost) = 0;
 
-  ExecutionState getHasMoreState();
+  [[nodiscard]] ExecutionState getHasMoreState();
 
   // TODO: Can we get rid of this? Problem: Subquery Executor is using it.
-  ExecutionNode const* getPlanNode() const;
+  [[nodiscard]] ExecutionNode const* getPlanNode() const;
 
-  transaction::Methods* transaction() const;
+  [[nodiscard]] velocypack::Options const* trxVpackOptions() const noexcept;
 
   /// @brief add a dependency
   void addDependency(ExecutionBlock* ep);
 
-  bool isInSplicedSubquery() const noexcept;
+  [[nodiscard]] bool isInSplicedSubquery() const noexcept;
 
  protected:
   /// @brief the execution engine
   ExecutionEngine* _engine;
 
-  /// @brief the transaction for this query
-  transaction::Methods* _trx;
+  velocypack::Options const* _trxVpackOptions;
 
   /// @brief the Result returned during the shutdown phase. Is kept for multiple
   ///        waiting phases.

diff --git a/arangod/Aql/ExecutionBlockImpl.cpp b/arangod/Aql/ExecutionBlockImpl.cpp
@@ -112,7 +112,7 @@ ExecutionBlockImpl<Executor>::ExecutionBlockImpl(ExecutionEngine* engine,
     : ExecutionBlock(engine, node),
       _dependencyProxy(_dependencies, engine->itemBlockManager(),
                        infos.getInputRegisters(), infos.numberOfInputRegisters(),
-                       transaction()->transactionContextPtr()->getVPackOptions()),
+                       trxVpackOptions()),
       _rowFetcher(_dependencyProxy),
       _infos(std::move(infos)),
       _executor(_rowFetcher, _infos),

diff --git a/arangod/Aql/ExecutionBlockImpl.h b/arangod/Aql/ExecutionBlockImpl.h
@@ -170,7 +170,7 @@ class ExecutionBlockImpl final : public ExecutionBlock {
 
   [[nodiscard]] std::pair<ExecutionState, Result> initializeCursor(InputAqlItemRow const& input) override;
 
-  Infos const& infos() const;
+  [[nodiscard]] Infos const& infos() const;
 
   /// @brief shutdown, will be called exactly once for the whole query
   /// Special implementation for all Executors that need to implement Shutdown

diff --git a/tests/Aql/ExecutionBlockImplTest.cpp b/tests/Aql/ExecutionBlockImplTest.cpp
@@ -36,6 +36,7 @@
 #include "Aql/ExecutionEngine.h"
 #include "Aql/Query.h"
 #include "Aql/SingleRowFetcher.h"
+#include "Transaction/Context.h"
 #include "Transaction/Methods.h"
 
 using namespace arangodb;
@@ -65,6 +66,10 @@ class ExecutionBlockImplTest : public ::testing::Test {
   fakeit::Mock<transaction::Methods> mockTrx;
   transaction::Methods& trx;
 
+  // Mock of the transaction context
+  fakeit::Mock<transaction::Context> mockContext;
+  transaction::Context& context;
+
   // Mock of the Query
   fakeit::Mock<Query> mockQuery;
   Query& query;
@@ -90,6 +95,7 @@ class ExecutionBlockImplTest : public ::testing::Test {
       : engine(mockEngine.get()),
         itemBlockManager(mockBlockManager.get()),
         trx(mockTrx.get()),
+        context(mockContext.get()),
         query(mockQuery.get()),
         lqueryOptions(mockQueryOptions.get()),
         profile(ProfileLevel(PROFILE_LEVEL_NONE)),
@@ -116,6 +122,9 @@ class ExecutionBlockImplTest : public ::testing::Test {
     fakeit::When(Method(mockQuery, trx)).AlwaysReturn(&trx);
 
     fakeit::When(Method(mockQueryOptions, getProfileLevel)).AlwaysReturn(profile);
+
+    fakeit::When(Method(mockTrx, transactionContextPtr)).AlwaysReturn(&context);
+    fakeit::When(Method(mockContext, getVPackOptions)).AlwaysReturn(&velocypack::Options::Defaults);
   }
 };
 
@@ -145,7 +154,7 @@ TEST_F(ExecutionBlockImplTest,
 }
 
 TEST_F(ExecutionBlockImplTest,
-       there_is_a_block_in_the_upstream_with_now_rows_inside_the_executor_waits_using_skipsome) {
+       there_is_a_block_in_the_upstream_with_no_rows_inside_the_executor_waits_using_skipsome) {
   std::deque<SharedAqlItemBlockPtr> blockDeque;
   SharedAqlItemBlockPtr block = buildBlock<1>(itemBlockManager, {{42}});
   blockDeque.push_back(std::move(block));

diff --git a/tests/Aql/WaitingExecutionBlockMock.cpp b/tests/Aql/WaitingExecutionBlockMock.cpp
@@ -25,8 +25,6 @@
 #include "Aql/AqlItemBlock.h"
 #include "Aql/ExecutionEngine.h"
 #include "Aql/ExecutionState.h"
-#include "Aql/ExecutionStats.h"
-#include "Aql/ExecutorInfos.h"
 #include "Aql/QueryOptions.h"
 
 #include <velocypack/velocypack-aliases.h>
@@ -92,24 +90,20 @@ std::pair<arangodb::aql::ExecutionState, size_t> WaitingExecutionBlockMock::skip
   traceSkipSomeBegin(atMost);
   if (!_hasWaited) {
     _hasWaited = true;
-    traceSkipSomeEnd(ExecutionState::WAITING, 0);
-    return {ExecutionState::WAITING, 0};
+    return traceSkipSomeEnd(ExecutionState::WAITING, 0);
   }
   _hasWaited = false;
 
   if (_data.empty()) {
-    traceSkipSomeEnd(ExecutionState::DONE, 0);
-    return {ExecutionState::DONE, 0};
+    return traceSkipSomeEnd(ExecutionState::DONE, 0);
   }
 
   size_t skipped = _data.front()->size();
   _data.pop_front();
 
   if (_data.empty()) {
-    traceSkipSomeEnd(ExecutionState::DONE, skipped);
-    return {ExecutionState::DONE, skipped};
+    return traceSkipSomeEnd(ExecutionState::DONE, skipped);
   } else {
-    traceSkipSomeEnd(ExecutionState::HASMORE, skipped);
-    return {ExecutionState::HASMORE, skipped};
+    return traceSkipSomeEnd(ExecutionState::HASMORE, skipped);
   }
 }