Testing with a newer hlint found a few minor issues; but all are real,
valid recommendations:

- don't use "if cond then f x else f y", but "f (if cond then x else y)"
- "if a then b else True" is equivalent to the simpler "not a || b"
- and as usual, one more ignore to our "testing basic properties"
  module

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Michael Hanselmann <hansmi@google.com>

This is very THH specific, and applies to all serialisations generated
by THH, so I'm adding it in its own module.

Probably we should add some more generic tests, but in general THH
code is tested by the various definitions; this new field type however
is not (yet), so this is why I want this specific unittest.

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Guido Trotter <ultrotter@google.com>

This patch converts all the call paths from 'Result' (which contains
just string errors) to 'ErrorResult', which holds
GanetiException-encoded errors. We can now return proper
OpPrereq/OpExec errors to the clients of the luxi/query socket.

The patch touches many files as we had to convert the entire call
chains in a single round. But it should be pretty straightforward
otherwise:

- change 'Result' into 'ErrorResult'
- add error annotations: change "Bad msg" into "Bad (XXXEror msg)"
- add a helper function for confd, where we don't send to client
  formatted exceptions, to convert back from ErrorResult into Result
- change tests similarly, where needed

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Michael Hanselmann <hansmi@google.com>

As described in the module doc string, while writing this it dawned
upon me that we're mixing all errors together into a single hierarchy
(well, type on the Haskell side), which is not good. Some errors are
used purely within noded, some in the CLI frontends, etc. so these
should not be the same type; frontend functions should only be able to
raise frontend errors, not backend ones.

As to this patch itself, I've used again Template Haskell to generate
both the data type and the serialisation functions, as the initial
version, hand-written, seemed too prone to errors due to string
matching.

A small unittest for checking serialisation consistency is also added.

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Guido Trotter <ultrotter@google.com>

This is just a bit of cleanup. The (.&&.) operator is internally just:

  a .&& b = conjoin [a, b]

so let's replace 'a .&&. b .&&. c .&&. d' directly with 'conjoin [a,
b, c, d]'.

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Guido Trotter <ultrotter@google.com>

After seeing how nice HUnit formats the error message on failed
'assertEqual', I think we can do better with ==?. Currently it says
(on one line): "Expected equality, but 1 /= 2".

This patch changes the code to format it similar to HUnit:

  Expected equality, but got mismatch
  expected: 1
   but got: 2

(on three lines). This makes it more clear what is the expected and
what is the wrong value.

A few tests have been modified to ensure that the expected value is
the second argument to ==?. This is different than HUnit, but makes
more sense in the operator version (I think).

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Helga Velroyen <helgav@google.com>

This includes:
 * The operator (/=?), which checks for inequality and prints
   an error message if it encounters equality. (Basically the
   negation of the (==?) operator).
 * Application of this operator in the test property
   prop_addPri_NoN1Fail.

Signed-off-by: Helga Velroyen <helgav@google.com>
Reviewed-by: Iustin Pop <iustin@google.com>

This patch includes:

* The 'failN1' flag is now only set if there is strictly less
  memory available than required for failover.
* Unit tests for that.

Signed-off-by: Helga Velroyen <helgav@google.com>
Reviewed-by: Iustin Pop <iustin@google.com>

Small simplifications of other unit tests using the (==?)
operator when possible, and typo fixes.

Signed-off-by: Helga Velroyen <helgav@google.com>
Reviewed-by: Iustin Pop <iustin@google.com>

Since we now have the GeneralResult as a multi-purpose monad, we can
remove the custom OpResult monad, and just use 'GeneralResult
FailMode' as our type. This allows removal of a few bits of
specialised infrastructure, relying instead on the generic one.

The restriction on using OpResult as a general monad remains as
before.

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Michael Hanselmann <hansmi@google.com>

Furthermore, a few messages have their capitalisation changed (fixed).

Signed-off-by: Helga Velroyen <helgav@google.com>
Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Iustin Pop <iustin@google.com>

Currently, our error monad—Result—has a plain string error type. This
is not good, as we don't have structured errors, we can't pass back
proper error information to Python code, etc.

To solve this, we generalise this type as 'GenericResult a', and make
Result an alias to 'GenericResult String' for compatibility with the
old code. New error hierarchies will be introduced as different
types. Furthermore, we generalise our helper functions too, so that
they can work on any 'GeneralInstance a' type, not only Result.

There are two small drawbacks to this generalisation. First, a Monad
instance requires (at least for the way we use it) a 'fail :: String
-> m a' instance, so we need to be able to build an 'a' value from a
string; therefore, we can implement the Monad instance only for a
newly-introduced typeclass, 'FromString', which requires the needed
conversion function. Second, due to the fact that 'String' is a type
alias (for [Char]) instead of an actual type, we need to enable the
FlexibleInstances language pragma; as far as I know, this has no
significant drawbacks.

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Michael Hanselmann <hansmi@google.com>

Before we reorganised the source tree, the 'Result' type was exported
from HTools/Types.hs. This changed during the reorg, but at that time
we didn't change the exports; instead, we kept re-exporting it from
the old module for compatibility.

In light of future changes to the Result type, let's stop this
re-export and cleanup the imports of the other modules.

One test is slightly rewritten with explicit types declaration (part
of the values already needed one, let's make it explicit).

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Michael Hanselmann <hansmi@google.com>

This patch cleans up duplicate code in Test.Ganeti.Query.Filter and
then adds a test for names consistency with Python's code behaviour
(stable ordering for simple filters and otherwise niceSort'ed
ordering).

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Michael Hanselmann <hansmi@google.com>

And associated unittests. This will be needed for classic-style
queries.

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Michael Hanselmann <hansmi@google.com>

This makes the "all" names queries consistent with the Python
results. The change requires updating the unittests, at which point a
duplicate error message is simplified.

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Michael Hanselmann <hansmi@google.com>

This replicates in the Haskell Query2 implementation the behaviour of
the Python code: if a "simple" filter is passed (one that contains
only Or aggregators  and EQ binary ops on the name field), then an
failure is flagged if the given values are not known.

Our implementation is pretty straightforward, with a few details:

- we ignore any NumericValues passed, since that inconsistency will be
  flagged by the filter compiler
- we return an the non-normalized names from the getRequestedNames
  function, and not the fully-normalized ones; this will be done later
  in individual query functions
- we test a few of the desired behaviours of the above-mentioned
  function

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Agata Murawska <agatamurawska@google.com>

Signed-off-by: Dato Simó <dato@google.com>
Reviewed-by: Iustin Pop <iustin@google.com>

This patch adds a NiceSort equivalent and the corresponding unittest
(partially copied from Python unittest). The difference between the
Python version and this one is that this implementation doesn't use
regular expressions, and as such it doesn't have the 8-groups
limitation.

The key-based version is separate from the non-key one (since we don't
have default arguments in Haskell), and is tested less in its absolute
properties but only that it is identical to the non-key version under
some transformations (the non-key version is much more tested).

This will be needed later in query name sorting.

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Michael Hanselmann <hansmi@google.com>

This defines the arguments supported and then modifies the
--help-completion output to include them too.

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Michael Hanselmann <hansmi@google.com>

This is a leftover from the times when we had a single, huge test
module; nowadays it's only an annoyance.

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Michael Hanselmann <hansmi@google.com>

This is, I believe, the last non-htools specific file that still lived
in the htools directory; it's already widely used in non-htools code,
so let's move it before we add more functionality to this module.

All changes are related to the name change, imports fixup, etc.; there
are no other changes in this patch.

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Michael Hanselmann <hansmi@google.com>

The tests we currently have assume, that all the data required for
running the query is available - once we add live data, this will no
longer be the case.

This patch adds boolean parameter to query function, which tells it
whether to ignore live parameters gathering.

Signed-off-by: Agata Murawska <agatamurawska@google.com>
Reviewed-by: Iustin Pop <iustin@google.com>

This adds tests similar to those used for node query. For now the
prop_queryGroup_noUnknown is disabled and commented out, as it is
fasifiable with ndparams and ipolicy. It may be removed or fixed
later on.

Also, prop_queryGrooup_types has one less property checked - it is
not the case that number of result rows should be equal to number
of nodes.

Signed-off-by: Agata Murawska <agatamurawska@google.com>
Reviewed-by: Iustin Pop <iustin@google.com>

This is a quite boring patch, just adding annotation information to
all existing options. Some of the annotations are not very good; but
we don't have support for more precise completion in
build-bash-completion, so this is good enough.

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Agata Murawska <agatamurawska@google.com>

Currently, we test and require that each individual program (hbal,
etc.) defines/supports the generic options (currently --help and
--version). Even with the test, this is not optimal, since it requires
changes in many places whenever we modify the list of generic options,
hence we move these out of the individual programs and into the
generic CLI handling code.

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Agata Murawska <agatamurawska@google.com>

We can build the test groups directly in the `testSuite' helper,
instead of doing it (much later) in the test harness.

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Guido Trotter <ultrotter@google.com>

Since the recent commits improved the speed of the two "slow" test
groups to regular test speed, we can remove this kludge and simplify
significantly our test runner, yay!

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Guido Trotter <ultrotter@google.com>

The Cluster object, as it is defined right now, has many '[String]'
members, which means that in a standard arbitrary generator these will
become very big, which is the reason for the current slowness of the
test 'Config_serialisation'.

By resizing the generator to 8 (arbitrary chosen to limit the list
length and the string sizes), and by reducing a bit the node count, we
can make this test be as fast as the others (about 10x
improvement). This means we can test more cases, for the same cost.

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Guido Trotter <ultrotter@google.com>

This test has a few deficiencies, which this patch addresses:

- using arbitrary 1 or 2 node count for allocation is obsolete,
  nowadays we need to use a number appropriate for the instance's disk
  template (and we should remove that parameter…)
- generating a random node is sub-optimal, since we could generate an
  offline node, and no instance will fit on a cluster composed of only
  offline nodes
- generating arbitrary instances "such that" they can be allocated is
  an expensive test; let's rather generate instances smaller than our
  template node, and add a check that they indeed can be allocated
- using boolean return type, instead of nicely annotated properties

For the nice annotation and the extra check, we need to change a
helper function's signature, so that we can extract a bit more
information out of it.

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Guido Trotter <ultrotter@google.com>

Currently, this test is very slow. Upon investigation, this is due to
how `tieredAlloc' works:

- tries to allocate one instance
- if failed, shrink the instance by the "most failed" resource
- restart

In this algorithm, if the "most failed" resource is e.g. memory, and
the maximum available memory is much smaller than the current
template, it means we will have to shrink and try to allocate many
many times until we finally get with the to-be-allocated instance
memory size to a reasonable value. In the real world, this is not the
case, but when testing with arbitrary memory/node values, it can be
that we execute the shrink hundreds of thousands of times per test.

So we "improve" the test by directly generating an instance just
slightly bigger than the node, so that we don't have to shrink too
many times. This requires a new export from test/…/Instance.hs.

Additionally, we allow up to 5 instances to be tiered-allocated, and
we cleanup the test checks, making the conditions much, much more
readable (IMHO).

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Guido Trotter <ultrotter@google.com>

This test expands the "single-alloc-no-rebalance" by allocating a few
instances on a small cluster, and ensuring that after we allocate all
of them, either we can't rebalance or if we rebalance the score
improvement is very small.

The last condition is needed because sometime rounding errors (we're
using double-precision floating point) can accumulate and result in
what is a no real change in the cluster state, but with an
infinitesimal score decrease (e.g. 1e-14).

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Guido Trotter <ultrotter@google.com>

Currently, this just checks that a cluster cannot be rebalanced after
a single instance allocation. However, we can also test whether the
allocation decision computed a correct new cluster score, by checking
that against the one computed from the actual new node list.

Also, for nicer display, we convert the test from a Boolean to a
Property, with nice annotations.

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: Guido Trotter <ultrotter@google.com>

Commit 2cdaf225, “Re-enable standard hlint warnings”, got it almost
right. The only problem is that (confusingly) the default set of hints
is not in HLint.Default, but in HLint.HLint (it includes Default and
some built-ins).

After changing the lint file to correctly include the defaults, we had
another 128 suggestions:

  - Error: Eta reduce (2)
  - Error: Redundant bracket (4)
  - Error: Redundant do (17)
  - Error: Redundant lambda (7)
  - Error: Redundant return (1)
  - Warning: Avoid lambda (2)
  - Warning: Redundant $ (42)
  - Warning: Redundant bracket (35)
  - Warning: Use : (1)
  - Warning: Use String (4)
  - Warning: Use camelCase (10)
  - Warning: Use section (3)

which are fixed by the current patch. Note that the 10 "Use camelCase"
were all due to hlint not “knowing” the idiom of ‘case_’ (it does for
‘prop_’), for which I filled
http://code.google.com/p/ndmitchell/issues/detail?id=558

.

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: René Nussbaumer <rn@google.com>

While investigating how we could test the Daemon.hs module, I realised
that we have a very, erm, sub-optimal situation:

- HTools/CLI.hs has a nice IO/pure separation testing in cmdline
  parsing, which allows some basic functionality to be tested, but
  uses direct 'read' in many options, which fails at runtime when
  evaluating the argument, and not when parsing the options
- Daemon.hs lacks that, but has a much nicer 'reqWithConversion'
  helper that can be used for nicer option parsing, and uses that +
  tryRead instead of plain 'read'

Since this situation is very bad, let's clean it up. We introduce yet
another module, Common.hs, that holds functionality common to all
command line programs (daemons or not). We move the parsing to this
module, and introduce a type class to handle option types which
support --help/--version. This allows removal of duplicated code from
CLI.hs and Daemon.hs.

The other part of the patch is cleanup/rework of the tests for this
code: we introduce some helpers (checkOpt, passFailOpt,
checkEarlyExit) that can be used from the much-slimmer now tests for
CLI and Daemon. In the common module, we just test the yes/no helper
we have. Many new tests for boolean options and numeric options are
added.

A side change is the removal of the obsolete `--replay-count',
`--test-size' options (unused since commit 95f6c931, “Switch Haskell
test harness to test-framework”).

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: René Nussbaumer <rn@google.com>

Found via the newly added unit-tests, which test most of the
serialisation code in Query/Language (except for QueryResult, for
which we already tests both sub-components separately).

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: René Nussbaumer <rn@google.com>

These tests are node specific only because we don't have other query
types implemented yet, but what they actually test is the various
filter types.

The tests are trying to cover most filter functionality; missing for
now is proper checking for ContainsFilter and TrueFilter, the rest
should be more or less covered.

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: René Nussbaumer <rn@google.com>

These new tests check that:

- no known fields return unknown
- any unknown field returns unknown
- the type of the fields is consistent between the getters and the
  field definition
- the length of each result row corresponds with the number of fields
  queried, and the length of the field definitions returned
- the length of the rows corresponds to the number of nodes
- querying fields on empty fields returns all fields

Finally this patch found a bug, in that the pinst_list/sinst_list
fields were declared as QFTNumber (copy-paste error from
pinst_cnt/sinst_cnt), yay!

I also changed genEmptyCluster to ensure that it generates unique node
names, so that the number of result rows is consistent with what we
requested, and switched ResultEntry from a normal constructor to
record syntax, so that we can extract the fields without having to use
pattern matching.

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: René Nussbaumer <rn@google.com>

This is symmetric to failTest, and allows us to use it in cases where
we need to return a property.

While replacing 'property True' with it, I saw a case where we can
simplify the use and thus reworked that check.

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: René Nussbaumer <rn@google.com>

Using the recently-added genArbitrary, we can now implement Arbitrary
instances for even "huge" objects like Cluster, so let's use that to
implement entire ConfigData serialisation tests.

Note that, as we don't have yet proper types for some of the Params
fields, we have to cheat via FlexibleInstances and
TypeSynonymInstances, using either empty items or real arbitrary
values.

Signed-off-by: Iustin Pop <iustin@google.com>
Reviewed-by: René Nussbaumer <rn@google.com>