On Mon, Oct 14, 2013 at 10:25:30AM +0200, Hrvoje Ribicic wrote: > As the retrieval of instance information is a tricky operation which > affects the way the status is calculated, a few tests have been added
Which status? The instance status? Present tense. > to ensure its behaviour remains constant. > > Signed-off-by: Hrvoje Ribicic <[email protected]> > --- > Makefile.am | 1 + > src/Ganeti/Query/Instance.hs | 1 + > test/hs/Test/Ganeti/Query/Instance.hs | 131 > ++++++++++++++++++++++++++++++++++ > test/hs/htest.hs | 2 + > 4 files changed, 135 insertions(+) > create mode 100644 test/hs/Test/Ganeti/Query/Instance.hs > > diff --git a/Makefile.am b/Makefile.am > index 4dcc6f9..bdffcaa 100644 > --- a/Makefile.am > +++ b/Makefile.am > @@ -755,6 +755,7 @@ HS_TEST_SRCS = \ > test/hs/Test/Ganeti/OpCodes.hs \ > test/hs/Test/Ganeti/Query/Aliases.hs \ > test/hs/Test/Ganeti/Query/Filter.hs \ > + test/hs/Test/Ganeti/Query/Instance.hs \ > test/hs/Test/Ganeti/Query/Language.hs \ > test/hs/Test/Ganeti/Query/Network.hs \ > test/hs/Test/Ganeti/Query/Query.hs \ > diff --git a/src/Ganeti/Query/Instance.hs b/src/Ganeti/Query/Instance.hs > index 67ae64f..6dc74f2 100644 > --- a/src/Ganeti/Query/Instance.hs > +++ b/src/Ganeti/Query/Instance.hs > @@ -27,6 +27,7 @@ module Ganeti.Query.Instance > ( Runtime > , fieldsMap > , collectLiveData > + , getInstanceInfo > , instanceFields > , instanceAliases > ) where > diff --git a/test/hs/Test/Ganeti/Query/Instance.hs > b/test/hs/Test/Ganeti/Query/Instance.hs > new file mode 100644 > index 0000000..83124e1 > --- /dev/null > +++ b/test/hs/Test/Ganeti/Query/Instance.hs > @@ -0,0 +1,131 @@ > +{-# LANGUAGE TemplateHaskell #-} Are you sure you need this ? > +{-# OPTIONS_GHC -fno-warn-orphans #-} Are you sure you need this ? > + > +{-| Unittests for Instance Queries. > + > +-} > + > +{- > + > +Copyright (C) 2013 Google Inc. > + > +This program is free software; you can redistribute it and/or modify > +it under the terms of the GNU General Public License as published by > +the Free Software Foundation; either version 2 of the License, or > +(at your option) any later version. > + > +This program is distributed in the hope that it will be useful, but > +WITHOUT ANY WARRANTY; without even the implied warranty of > +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > +General Public License for more details. > + > + Extra empty line. > +You should have received a copy of the GNU General Public License > +along with this program; if not, write to the Free Software > +Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA > +02110-1301, USA. > + > +-} > + > +module Test.Ganeti.Query.Instance > + ( testQuery_Instance > + ) where > + > +import qualified Data.Map as Map > +import qualified Data.Set as Set > + > +import Ganeti.JSON > +import Ganeti.Objects > +import Ganeti.Query.Instance > +import Ganeti.Rpc > +import Ganeti.Types > + > +import Test.Ganeti.TestHelper > + > +import Test.HUnit The Ganeti style guide says that the imports should be grouped in 3 sections: 1. standard imports 2. external parties 3. local imports > + > +{-# ANN module "HLint: ignore Use camelCase" #-} > + > +-- | Creates an instance with the desired name, pnode uuid, and AdminState. > +-- All other fields are placeholders. > +createInstance :: String -> String -> AdminState -> Instance > +createInstance name pnodeUuid adminState = > + Instance name pnodeUuid "" Kvm > + (GenericContainer . Map.fromList $ []) Map.empty > + (PartialBeParams Nothing Nothing Nothing Nothing Nothing Nothing) > + (GenericContainer . Map.fromList $ []) Map.empty > + adminState [] [] DTDrbd8 False Nothing 0.0 0.0 "" 0 Set.empty > + > +-- | A fake InstanceInfo to be used to check values. > +fakeInstanceInfo :: InstanceInfo > +fakeInstanceInfo = InstanceInfo 0 "" 0 0 > + > +-- | Erroneous node response - the exact error does not matter. > +responseError :: String -> (String, ERpcError a) > +responseError name = (name, Left . RpcResultError $ "Insignificant error") > + > +-- | Successful response - the error does not really matter. > +responseSuccess :: String > + -> [String] > + -> (String, ERpcError RpcResultAllInstancesInfo) > +responseSuccess name instNames = (name, Right . > + RpcResultAllInstancesInfo . map (\x -> (x, fakeInstanceInfo)) $ instNames) Use tuple sections ... map (, fakeInstanceInfo) ... > + > +-- | The instance used for testing. Called Waldo as test cases involve > trouble > +-- finding information related to it. > +waldoInstance :: Instance > +waldoInstance = createInstance "Waldo" "prim" AdminUp Now we know where Waldo is :) > + > +-- | Check that an error is thrown when the node is offline > +case_nodeOffline :: Assertion > +case_nodeOffline = > + let responses = [ responseError "prim" > + , responseError "second" > + , responseSuccess "node" ["NotWaldo", > "DefinitelyNotWaldo"] ] List indentation [ x , y ] > + in case getInstanceInfo responses waldoInstance of > + Left _ -> return () > + Right _ -> assertFailure "No error when info missing and node > offline" 1 space instead of 2. > + > +-- | Check that a Right Nothing is returned when the node is online, yet no > info > +-- is present anywhere in the system. > +case_nodeOnlineNoInfo :: Assertion > +case_nodeOnlineNoInfo = > + let responses = [ responseSuccess "prim" ["NotWaldo1"] > + , responseSuccess "second" ["NotWaldo2"] > + , responseError "node" ] > + in case getInstanceInfo responses waldoInstance of > + Left _ -> assertFailure > + "Error found when info missing but node online" > + Right Nothing -> return () > + Right _ -> assertFailure "Value found when info missing" > + > +-- | Check the case when the info is on the primary node > +case_infoOnPrimary :: Assertion > +case_infoOnPrimary = > + let responses = [ responseSuccess "prim" ["NotWaldo1", "Waldo"] > + , responseSuccess "second" ["NotWaldo2"] > + , responseSuccess "node" ["NotWaldo3"] ] > + in case getInstanceInfo responses waldoInstance of > + Left _ -> assertFailure "Error found when info > present" As a general comment about the error messages in this module: Remember that people in the team are going to be running your tests. If these tests fail, the error message is pretty much the starting point for them. Therefore, a good error message can make a huge difference. And remember also that they are not as familiar with this code as you are. Just something for you to think about. Personally, I don't think that 'Error found when info present' explains much about the cause of the error. > + Right (Just (_, True)) -> return () > + Right _ -> assertFailure "Value on primary but not > found" > + > +-- | Check the case when the info is on the primary node > +case_infoOnSecondary :: Assertion > +case_infoOnSecondary = > + let responses = [ responseSuccess "prim" ["NotWaldo1"] > + , responseSuccess "second" ["Waldo", "NotWaldo2"] > + , responseError "node" ] > + in case getInstanceInfo responses waldoInstance of > + Left _ -> assertFailure "Error found when info > present" > + Right (Just (_, False)) -> return () > + Right _ -> assertFailure > + "Value on secnodary but not found" Typo. > + > + Extra empty line. > +testSuite "Query_Instance" > + [ 'case_nodeOffline, > + 'case_nodeOnlineNoInfo, > + 'case_infoOnPrimary, > + 'case_infoOnSecondary > + ] List format. Thanks, Jose > diff --git a/test/hs/htest.hs b/test/hs/htest.hs > index 9c23f6e..e8f41c6 100644 > --- a/test/hs/htest.hs > +++ b/test/hs/htest.hs > @@ -60,6 +60,7 @@ import Test.Ganeti.Objects > import Test.Ganeti.OpCodes > import Test.Ganeti.Query.Aliases > import Test.Ganeti.Query.Filter > +import Test.Ganeti.Query.Instance > import Test.Ganeti.Query.Language > import Test.Ganeti.Query.Network > import Test.Ganeti.Query.Query > @@ -121,6 +122,7 @@ allTests = > , testOpCodes > , testQuery_Aliases > , testQuery_Filter > + , testQuery_Instance > , testQuery_Language > , testQuery_Network > , testQuery_Query > -- > 1.8.4 > -- Jose Antonio Lopes Ganeti Engineering Google Germany GmbH Dienerstr. 12, 80331, München Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Geschäftsführer: Graham Law, Christine Elizabeth Flores Steuernummer: 48/725/00206 Umsatzsteueridentifikationsnummer: DE813741370
