Skip to content

Conversation

gaborigloi
Copy link
Contributor

@gaborigloi gaborigloi commented Feb 27, 2018

We've recently started porting xapi's unit tests from OUnit, to Alcotest.

In #3402 we've added support for unit testing xapi using alcotest. Now there are two suites, one using ounit and another one using alcotest. New unit tests should use alcotest.

Moving to alcotest is beneficial because it has better output, thereby reducing log spam and making it easier to fix and write unit tests, and is probably lighter, more actively maintained, and more commonly used.

To get the full benefits of alcotest, we'd have to port all of our unit tests to it.

This PR ports some xapi unit tests from OUnit to Alcotest.

Useful links:

Signed-off-by: Gabor Igloi gabor.igloi@citrix.com

@coveralls
Copy link

coveralls commented Feb 27, 2018

Coverage Status

Coverage decreased (-0.03%) to 18.09% when pulling ee03ff6 on gaborigloi:alcotest into 3507094 on xapi-project:master.

Copy link
Contributor

@minishrink minishrink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall, just a couple of questions but otherwise good to merge.

let __context = Mock.make_context_with_new_db "Mock context" in
let licensed = try Xapi_vm_migrate.assert_licensed_storage_motion ~__context; true
with _ -> false in
assert_bool "Not licensed for SXM" licensed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this test no longer valid or is this function tested elsewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to simply delete this because it was not run: at line 32, we have skip "TODO"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth removing the skip "TODO" so it runs and including it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know whether it works or not, but probably not because it was skipped. My thinking was that it's probably not worth trying to debug old legacy tests 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI all the tests in Test_basic are really just demo tests that were written when unit tests first became a thing - they're not particularly useful as actual tests of the xapi codebase.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh ok, thanks a lot for the info @johnelse - then I'll just remove it completely 😄

assert_equal !Mock_daemon.times_called_stop 1
Alcotest.(check int) "times_called_start" !Mock_daemon.times_called_start 0;
Alcotest.(check int) "time_until_stopped" !Mock_daemon.times_called_stop 1

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I notice that lines 142-143 are repeated 7 times with different numbers, is it worth making an alias function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, done

Signed-off-by: Gabor Igloi <gabor.igloi@citrix.com>
Signed-off-by: Gabor Igloi <gabor.igloi@citrix.com>
Signed-off-by: Gabor Igloi <gabor.igloi@citrix.com>
These are just some demo unit tests that do not tests any useful
functionality of our code.

Signed-off-by: Gabor Igloi <gabor.igloi@citrix.com>
@gaborigloi gaborigloi merged commit 694b31b into xapi-project:master Mar 1, 2018
@gaborigloi gaborigloi deleted the alcotest branch March 1, 2018 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants