-
Notifications
You must be signed in to change notification settings - Fork 292
Resurrect a database unit test #3456
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Jon Ludlam <jonathan.ludlam@citrix.com>
ocaml/database/jbuild
Outdated
|
||
(executable | ||
((name database_server_main) | ||
(public_name database_server) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you set a public name it'll get installed, is that what you intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, copy-pasta mistake - thanks!
let find name t = | ||
try | ||
List.find (fun col -> col.Column.name = name) t.columns | ||
with Not_found -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks it is always annoying when you get Not_found
in the logs not knowing what was not found...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some minor comments. It's a giant test that you resurrected there!!
@@ -1,3 +1,6 @@ | |||
open Xapi_stdext_threads | |||
open Xapi_stdext_unix | |||
open Xapi_stdext_threads |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already opened above
ocaml/database/database_test.ml
Outdated
|
||
|
||
open Xapi_stdext_pervasives | ||
open Xapi_stdext_monadic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is opened above
let find name t = | ||
try | ||
List.find (fun col -> col.Column.name = name) t.columns | ||
with Not_found -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed!
ocaml/database/test_schemas.ml
Outdated
Schema.Table.name = "VM"; | ||
columns = [ _ref; uuid; name_label; vbds; pp; name_description; tags; other_config ]; | ||
columns = [ _ref; uuid; name_label; vbds; pp; | ||
name_description; tags; other_config ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you really mean to split them like this?
Signed-off-by: Jon Ludlam <jonathan.ludlam@citrix.com>
Signed-off-by: Jon Ludlam <jonathan.ludlam@citrix.com>
Signed-off-by: Jon Ludlam <jonathan.ludlam@citrix.com>
then failwith (Printf.sprintf "check_many_to_many: bar(bar:1).foos expected ('foo:1') got %s" (Schema.Value.marshal bar_foos)); | ||
|
||
(* set foo.bars to [] *) | ||
(* let foo_1 = Table.find "foo:1" (TableSet.find "foo" (Database.tableset db)) in*) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why you left this line commented out?
|
||
|
||
|
||
() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The choice of spacing and newlines are "interesting" in some part of this code 😜
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I've forgotten to ocp-indent it.
ocaml/database/database_test.ml
Outdated
|
||
(* let xs = Client.read_set_ref t where_name_label in | ||
if not (List.mem name xs) | ||
then failwith "read_set_ref <valid table> <valid return> <valid field> <valid value>"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why you disabled this subtest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a couple of questions concerning two parts of the tests that are commented, otherwise this looks like a good thing to have (and +0.5% in xapi coverage... wow, that's huge)
Signed-off-by: Jon Ludlam <jonathan.ludlam@citrix.com>
Signed-off-by: Jon Ludlam jonathan.ludlam@citrix.com