LGTM. Thanks, Jose
On Tue, Oct 15, 2013 at 07:20:30PM +0200, Hrvoje Ribicic wrote: > 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 -- 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
