On Tue, Oct 15, 2013 at 10:09 AM, Jose A. Lopes <[email protected]>wrote:
> 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.
>
>
The instance status, specified it, and fixed the tense.
>
> > ( 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 ?
>
>
No and no. Fixed, thanks!
>
>
> Extra empty line.
>
>
Removed.
> > +
> > +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
>
>
My apologies, I saw that the test categories had been grouped separately in
other files (e.g. Network.hs in the same folder).
Fixed now.
> > +
> > +{-# 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
Fixed.
> > + 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) ...
>
>
Done!
> > +
> > +-- | 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 :)
>
> What, but not where!
> > +
> > +-- | 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
> ]
>
> Fixed.
> > + in case getInstanceInfo responses waldoInstance of
> > + Left _ -> return ()
> > + Right _ -> assertFailure "No error when info missing and node
> offline"
>
> 1 space instead of 2.
>
>
Fixed.
> 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.
>
>
The error values have been improved.
> > + 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.
>
Fixed.
>
> > +
> > +
>
> Extra empty line.
>
Fixed.
>
> > +testSuite "Query_Instance"
> > + [ 'case_nodeOffline,
> > + 'case_nodeOnlineNoInfo,
> > + 'case_infoOnPrimary,
> > + 'case_infoOnSecondary
> > + ]
>
> List format.
>
Fixed.
Thanks for the review, the interdiff is:
diff --git a/test/hs/Test/Ganeti/Query/Instance.hs
b/test/hs/Test/Ganeti/Query/Instance.hs
index 83124e1..ff0491a 100644
--- a/test/hs/Test/Ganeti/Query/Instance.hs
+++ b/test/hs/Test/Ganeti/Query/Instance.hs
@@ -1,5 +1,4 @@
-{-# LANGUAGE TemplateHaskell #-}
-{-# OPTIONS_GHC -fno-warn-orphans #-}
+{-# LANGUAGE TupleSections #-}
{-| Unittests for Instance Queries.
@@ -41,7 +40,6 @@ import Ganeti.Rpc
import Ganeti.Types
import Test.Ganeti.TestHelper
-
import Test.HUnit
{-# ANN module "HLint: ignore Use camelCase" #-}
@@ -51,9 +49,9 @@ import Test.HUnit
createInstance :: String -> String -> AdminState -> Instance
createInstance name pnodeUuid adminState =
Instance name pnodeUuid "" Kvm
- (GenericContainer . Map.fromList $ [])
+ (GenericContainer Map.empty)
(PartialBeParams Nothing Nothing Nothing Nothing Nothing Nothing)
- (GenericContainer . Map.fromList $ [])
+ (GenericContainer Map.empty)
adminState [] [] DTDrbd8 False Nothing 0.0 0.0 "" 0 Set.empty
-- | A fake InstanceInfo to be used to check values.
@@ -69,7 +67,7 @@ responseSuccess :: String
-> [String]
-> (String, ERpcError RpcResultAllInstancesInfo)
responseSuccess name instNames = (name, Right .
- RpcResultAllInstancesInfo . map (\x -> (x, fakeInstanceInfo)) $
instNames)
+ RpcResultAllInstancesInfo . map (, fakeInstanceInfo) $ instNames)
-- | The instance used for testing. Called Waldo as test cases involve
trouble
-- finding information related to it.
@@ -81,10 +79,12 @@ case_nodeOffline :: Assertion
case_nodeOffline =
let responses = [ responseError "prim"
, responseError "second"
- , responseSuccess "node" ["NotWaldo",
"DefinitelyNotWaldo"] ]
+ , responseSuccess "node" ["NotWaldo",
"DefinitelyNotWaldo"]
+ ]
in case getInstanceInfo responses waldoInstance of
Left _ -> return ()
- Right _ -> assertFailure "No error when info missing and node
offline"
+ Right _ -> assertFailure
+ "Error occurred when instance info is missing and node is offline"
-- | Check that a Right Nothing is returned when the node is online, yet
no info
-- is present anywhere in the system.
@@ -92,40 +92,46 @@ case_nodeOnlineNoInfo :: Assertion
case_nodeOnlineNoInfo =
let responses = [ responseSuccess "prim" ["NotWaldo1"]
, responseSuccess "second" ["NotWaldo2"]
- , responseError "node" ]
+ , 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"
+ Left _ -> assertFailure
+ "Error occurred when instance info could be found on primary"
+ Right Nothing -> return ()
+ Right _ -> assertFailure
+ "Some instance info found when none should be"
-- | 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"] ]
+ , responseSuccess "node" ["NotWaldo3"]
+ ]
in case getInstanceInfo responses waldoInstance of
- Left _ -> assertFailure "Error found when info
present"
- Right (Just (_, True)) -> return ()
- Right _ -> assertFailure "Value on primary but not
found"
+ Left _ -> assertFailure
+ "Cannot retrieve instance info when present on primary node"
+ Right (Just (_, True)) -> return ()
+ Right _ -> assertFailure
+ "Instance info not found on primary node, despite being there"
-- | 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" ]
+ , 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"
-
+ Left _ -> assertFailure
+ "Cannot retrieve instance info when present on secondary node"
+ Right (Just (_, False)) -> return ()
+ Right _ -> assertFailure
+ "Instance info not found on secondary node, despite being there"
testSuite "Query_Instance"
- [ 'case_nodeOffline,
- 'case_nodeOnlineNoInfo,
- 'case_infoOnPrimary,
- 'case_infoOnSecondary
+ [ 'case_nodeOffline
+ , 'case_nodeOnlineNoInfo
+ , 'case_infoOnPrimary
+ , 'case_infoOnSecondary
]
Hrvoje Ribicic
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
