Yoav Kleinberger has uploaded a new change for review.
Change subject: tests: alignmentScanTests.py is a functional test ......................................................................
tests: alignmentScanTests.py is a functional test
Since alignmentScanTests invokes actual storage code, and does not mock it out, it should be categorized as a functional test.
Change-Id: I4e20d17c3ebee1203bb5a721ce44d5867570ce8e Signed-off-by: Yoav Kleinberger ykleinbe@redhat.com --- M tests/Makefile.am M tests/functional/Makefile.am R tests/functional/alignmentScanTests.py 3 files changed, 1 insertion(+), 1 deletion(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/45/29745/1
diff --git a/tests/Makefile.am b/tests/Makefile.am index 4ef8f7d..f7f4f97 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -23,7 +23,6 @@ SUBDIRS = functional
test_modules = \ - alignmentScanTests.py \ blocksdTests.py \ bridgeTests.py \ cPopenTests.py \ diff --git a/tests/functional/Makefile.am b/tests/functional/Makefile.am index b87fdf0..4fc2cc8 100644 --- a/tests/functional/Makefile.am +++ b/tests/functional/Makefile.am @@ -33,6 +33,7 @@ vmRecoveryTests.py \ storageTests.py \ veth.py \ + alignmentScanTests.py \ $(NULL)
dist_vdsmfunctests_DATA = \ diff --git a/tests/alignmentScanTests.py b/tests/functional/alignmentScanTests.py similarity index 100% rename from tests/alignmentScanTests.py rename to tests/functional/alignmentScanTests.py
oVirt Jenkins CI Server has posted comments on this change.
Change subject: tests: alignmentScanTests.py is a functional test ......................................................................
Patch Set 1:
Build Successful
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/10091/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/10876/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/11033/ : SUCCESS
Yoav Kleinberger has posted comments on this change.
Change subject: tests: alignmentScanTests.py is a functional test ......................................................................
Patch Set 1: Verified+1
Vered Volansky has posted comments on this change.
Change subject: tests: alignmentScanTests.py is a functional test ......................................................................
Patch Set 1: Code-Review+1
Dan Kenigsberg has posted comments on this change.
Change subject: tests: alignmentScanTests.py is a functional test ......................................................................
Patch Set 1: Code-Review-1
(1 comment)
http://gerrit.ovirt.org/#/c/29745/1//COMMIT_MSG Commit Message:
Line 6: Line 7: tests: alignmentScanTests.py is a functional test Line 8: Line 9: Since alignmentScanTests invokes actual storage code, and does not mock Line 10: it out, it should be categorized as a functional test. The functional tests are special since they test the functionality of Vdsm via its API. They require a running and responsive Vdsm instance.
This is not the case here - this module checks (actually, only SHOULD check, as it is buggy) that and internal scanImage() function does what it should. Line 11: Line 12: Change-Id: I4e20d17c3ebee1203bb5a721ce44d5867570ce8e
Nir Soffer has posted comments on this change.
Change subject: tests: alignmentScanTests.py is a functional test ......................................................................
Patch Set 1:
(1 comment)
http://gerrit.ovirt.org/#/c/29745/1//COMMIT_MSG Commit Message:
Line 6: Line 7: tests: alignmentScanTests.py is a functional test Line 8: Line 9: Since alignmentScanTests invokes actual storage code, and does not mock Line 10: it out, it should be categorized as a functional test.
The functional tests are special since they test the functionality of Vdsm
This is not a unit test, because it is slow and mostly does not test our code. We may need different directory for such tests. Line 11: Line 12: Change-Id: I4e20d17c3ebee1203bb5a721ce44d5867570ce8e
Yoav Kleinberger has posted comments on this change.
Change subject: tests: alignmentScanTests.py is a functional test ......................................................................
Patch Set 1:
(1 comment)
http://gerrit.ovirt.org/#/c/29745/1//COMMIT_MSG Commit Message:
Line 7: tests: alignmentScanTests.py is a functional test Line 8: Line 9: Since alignmentScanTests invokes actual storage code, and does not mock Line 10: it out, it should be categorized as a functional test. Line 11: I agree with Nir.
We used to call these "whitebox" tests - they actually run code in libraries (write to storage, open sockets, whatever) on the one hand - so they are not unit tests. On the other hand the test author knows and refers to what goes on inside - so it's not a blackbox test. Line 12: Change-Id: I4e20d17c3ebee1203bb5a721ce44d5867570ce8e
Yoav Kleinberger has posted comments on this change.
Change subject: tests: introducing whitebox tests ......................................................................
Patch Set 2: Verified+1
oVirt Jenkins CI Server has posted comments on this change.
Change subject: tests: introducing whitebox tests ......................................................................
Patch Set 2:
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/10349/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/11134/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/11291/ : FAILURE
Alon Bar-Lev has posted comments on this change.
Change subject: tests: introducing whitebox tests ......................................................................
Patch Set 2:
(4 comments)
http://gerrit.ovirt.org/#/c/29745/2//COMMIT_MSG Commit Message:
Line 18: because they are aware of the inside implementation (as opposed to Line 19: blackbox tests), but they check that things "really work" (e.g. Line 20: libraries get called, sockets are opened, etc.) , and not just Line 21: the internal logic of some small section of code (which would Line 22: characterize a unit test). it this is the reason, the test can be explicitly enabled by using test framework argument, no? if this is the case, why separate it into own directly? Line 23: Line 24: Change-Id: I4e20d17c3ebee1203bb5a721ce44d5867570ce8e
http://gerrit.ovirt.org/#/c/29745/2/tests/Makefile.am File tests/Makefile.am:
Line 21: include $(top_srcdir)/build-aux/Makefile.subs Line 22: Line 23: SUBDIRS = \ Line 24: functional \ Line 25: whitebox while you at it, please add $(NULL) at last line Line 26: Line 27: test_modules = \ Line 28: blocksdTests.py \ Line 29: bridgeTests.py \
http://gerrit.ovirt.org/#/c/29745/2/tests/whitebox/Makefile.am File tests/whitebox/Makefile.am:
Line 1: # Line 2: # Copyright 2012-2013 Red Hat, Inc. fix Line 3: # Line 4: # This program is free software; you can redistribute it and/or modify Line 5: # it under the terms of the GNU General Public License as published by Line 6: # the Free Software Foundation; either version 2 of the License, or
Line 17: # Line 18: # Refer to the README and COPYING files for full details of the license Line 19: # Line 20: Line 21: vdsmwhiteboxtestsdir = ${vdsmtestsdir}/whitebox $() instead of ${}
but why are we installing these?
if we do not need to install, please use:
dist_noinst_PYTHON = Line 22: Line 23: dist_vdsmwhiteboxtests_PYTHON = \ Line 24: alignmentScanTests.py
Yoav Kleinberger has posted comments on this change.
Change subject: tests: introducing whitebox tests ......................................................................
Patch Set 2:
(1 comment)
http://gerrit.ovirt.org/#/c/29745/2//COMMIT_MSG Commit Message:
Line 18: because they are aware of the inside implementation (as opposed to Line 19: blackbox tests), but they check that things "really work" (e.g. Line 20: libraries get called, sockets are opened, etc.) , and not just Line 21: the internal logic of some small section of code (which would Line 22: characterize a unit test).
it this is the reason, the test can be explicitly enabled by using test fra
I assume you meant its own directory.
The way I see it, directory structure is meant to convey meaning. So I would actually like to have test/unit, test/whitebox and test/functional
You could convey this meaning by documenting in Makefiles, but they are rather unreadable as it is. Line 23: Line 24: Change-Id: I4e20d17c3ebee1203bb5a721ce44d5867570ce8e
Alon Bar-Lev has posted comments on this change.
Change subject: tests: introducing whitebox tests ......................................................................
Patch Set 2:
(1 comment)
http://gerrit.ovirt.org/#/c/29745/2//COMMIT_MSG Commit Message:
Line 18: because they are aware of the inside implementation (as opposed to Line 19: blackbox tests), but they check that things "really work" (e.g. Line 20: libraries get called, sockets are opened, etc.) , and not just Line 21: the internal logic of some small section of code (which would Line 22: characterize a unit test).
I assume you meant its own directory.
you can put README within test/functional with set of variable to enable/disable to gain functionality X Line 23: Line 24: Change-Id: I4e20d17c3ebee1203bb5a721ce44d5867570ce8e
Yoav Kleinberger has posted comments on this change.
Change subject: tests: introducing whitebox tests ......................................................................
Patch Set 2:
(1 comment)
http://gerrit.ovirt.org/#/c/29745/2//COMMIT_MSG Commit Message:
Line 18: because they are aware of the inside implementation (as opposed to Line 19: blackbox tests), but they check that things "really work" (e.g. Line 20: libraries get called, sockets are opened, etc.) , and not just Line 21: the internal logic of some small section of code (which would Line 22: characterize a unit test).
you can put README within test/functional with set of variable to enable/di
why would I put a non-functional test in a dir called 'functional', and then use a README to compensate for this confusion Line 23: Line 24: Change-Id: I4e20d17c3ebee1203bb5a721ce44d5867570ce8e
Yoav Kleinberger has posted comments on this change.
Change subject: tests: introducing whitebox tests ......................................................................
Patch Set 2:
(2 comments)
http://gerrit.ovirt.org/#/c/29745/2/tests/whitebox/Makefile.am File tests/whitebox/Makefile.am:
Line 1: # Line 2: # Copyright 2012-2013 Red Hat, Inc.
fix
Done Line 3: # Line 4: # This program is free software; you can redistribute it and/or modify Line 5: # it under the terms of the GNU General Public License as published by Line 6: # the Free Software Foundation; either version 2 of the License, or
Line 17: # Line 18: # Refer to the README and COPYING files for full details of the license Line 19: # Line 20: Line 21: vdsmwhiteboxtestsdir = ${vdsmtestsdir}/whitebox
$() instead of ${}
Done Line 22: Line 23: dist_vdsmwhiteboxtests_PYTHON = \ Line 24: alignmentScanTests.py
Yoav Kleinberger has posted comments on this change.
Change subject: tests: introducing whitebox tests ......................................................................
Patch Set 3: Verified+1
oVirt Jenkins CI Server has posted comments on this change.
Change subject: tests: introducing whitebox tests ......................................................................
Patch Set 3:
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/10356/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/11141/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/11298/ : FAILURE
Alon Bar-Lev has posted comments on this change.
Change subject: tests: introducing whitebox tests ......................................................................
Patch Set 3: Code-Review+1
+1 for autotools usage
not sure about the usage of tests that can be run on specific environment only... hey are not helping anyone at upstream but you, so maybe just not publish these tests.
Yoav Kleinberger has posted comments on this change.
Change subject: tests: introducing whitebox tests ......................................................................
Patch Set 3:
They are already published. I haven't changed the test itself, just its categorization.
Alon Bar-Lev has posted comments on this change.
Change subject: tests: introducing whitebox tests ......................................................................
Patch Set 3:
They are already published. I haven't changed the test itself, just its categorization.
which means that we need to stop and think of correct solution, nothing should be taken for granted.
a solution might be to remove this entirely.
Allon Mureinik has posted comments on this change.
Change subject: tests: introducing whitebox tests ......................................................................
Patch Set 3:
@Alon - IMHO, it's a good test, albeit misplaced. Why would we want to remove it?
Alon Bar-Lev has posted comments on this change.
Change subject: tests: introducing whitebox tests ......................................................................
Patch Set 3:
@Alon - IMHO, it's a good test, albeit misplaced. Why would we want to remove it?
who can run it? is there a setup script for the environment? if no, what is the benefit for upstream?
you can have your own group personal repo with lots of internal tests, that's fine. but whatever is published upstream should be usable for other people as well.
I know nothing of this test... just raising the above...
Nir Soffer has posted comments on this change.
Change subject: tests: introducing whitebox tests ......................................................................
Patch Set 3:
I like the concept of moving these tests from the regular test suit, making it easier to run the regular suit without special flags.
But I don't understand the whitebox name - this is not a whitebox test, this simply a small functional test, or integration (with libguestfs) test.
So I would name this directory "integration" or something else.
Yoav Kleinberger has posted comments on this change.
Change subject: tests: introducing whitebox tests ......................................................................
Patch Set 3:
This is how we used the term 'whitebox' at LiveU. From Wikipedia, whitebox is a quite wide-ranging term. It *could* be used in this way.
If you have a better name than "integration" - which it is not (unless you count integration with external libraries...), let's hear it.
Allon Mureinik has posted comments on this change.
Change subject: tests: introducing whitebox tests ......................................................................
Patch Set 3: Code-Review+1
Alon - If I get your point correctly, what's missing here is documentation - a list of requirements and a "user manual" for running these test - Yoav, can you add a couple a lines in tests/README ?
Yoav Kleinberger has posted comments on this change.
Change subject: tests: introducing whitebox tests ......................................................................
Patch Set 4: Verified+1
Added a description in the tests/README for completeness
Alon Bar-Lev has posted comments on this change.
Change subject: tests: introducing whitebox tests ......................................................................
Patch Set 4:
(1 comment)
http://gerrit.ovirt.org/#/c/29745/4/tests/README File tests/README:
Line 35: i.e. they actually open sockets, read files, write to storage, etc. This Line 36: contrasts with a unit test, in which external components should be mocked. It Line 37: also contrasts with the functional tests, in that the scope of code checked is Line 38: much smaller, and the test is aware of the underlying implementation (hence the Line 39: term "white" box, as opposed to "black" box). so everyone can run these without special setup? Line 40:
oVirt Jenkins CI Server has posted comments on this change.
Change subject: tests: introducing whitebox tests ......................................................................
Patch Set 4:
Build Successful
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/10461/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/11246/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/11403/ : SUCCESS
Yoav Kleinberger has posted comments on this change.
Change subject: tests: introducing whitebox tests ......................................................................
Patch Set 4:
(1 comment)
http://gerrit.ovirt.org/#/c/29745/4/tests/README File tests/README:
Line 35: i.e. they actually open sockets, read files, write to storage, etc. This Line 36: contrasts with a unit test, in which external components should be mocked. It Line 37: also contrasts with the functional tests, in that the scope of code checked is Line 38: much smaller, and the test is aware of the underlying implementation (hence the Line 39: term "white" box, as opposed to "black" box).
so everyone can run these without special setup?
Yes. Any machine that can run VDSM can run these tests. Line 40:
Alon Bar-Lev has posted comments on this change.
Change subject: tests: introducing whitebox tests ......................................................................
Patch Set 4:
(1 comment)
http://gerrit.ovirt.org/#/c/29745/4/tests/README File tests/README:
Line 35: i.e. they actually open sockets, read files, write to storage, etc. This Line 36: contrasts with a unit test, in which external components should be mocked. It Line 37: also contrasts with the functional tests, in that the scope of code checked is Line 38: much smaller, and the test is aware of the underlying implementation (hence the Line 39: term "white" box, as opposed to "black" box).
Yes. Any machine that can run VDSM can run these tests.
so it is indeed integration test. Line 40:
Nir Soffer has posted comments on this change.
Change subject: tests: introducing whitebox tests ......................................................................
Patch Set 4:
(1 comment)
http://gerrit.ovirt.org/#/c/29745/4/tests/README File tests/README:
Line 30: ---------------------- Line 31: Line 32: Whitebox tests are somewhere in the middle between unit tests and functional Line 33: tests (see above). A whitebox test checks functionality of a small number Line 34: (perhaps one) of components, in the sense that these components "reall work", reall -> really Line 35: i.e. they actually open sockets, read files, write to storage, etc. This Line 36: contrasts with a unit test, in which external components should be mocked. It Line 37: also contrasts with the functional tests, in that the scope of code checked is Line 38: much smaller, and the test is aware of the underlying implementation (hence the
Yoav Kleinberger has posted comments on this change.
Change subject: tests: introducing whitebox tests ......................................................................
Patch Set 4:
(2 comments)
http://gerrit.ovirt.org/#/c/29745/4/tests/README File tests/README:
Line 30: ---------------------- Line 31: Line 32: Whitebox tests are somewhere in the middle between unit tests and functional Line 33: tests (see above). A whitebox test checks functionality of a small number Line 34: (perhaps one) of components, in the sense that these components "reall work",
reall -> really
Done Line 35: i.e. they actually open sockets, read files, write to storage, etc. This Line 36: contrasts with a unit test, in which external components should be mocked. It Line 37: also contrasts with the functional tests, in that the scope of code checked is Line 38: much smaller, and the test is aware of the underlying implementation (hence the
Line 35: i.e. they actually open sockets, read files, write to storage, etc. This Line 36: contrasts with a unit test, in which external components should be mocked. It Line 37: also contrasts with the functional tests, in that the scope of code checked is Line 38: much smaller, and the test is aware of the underlying implementation (hence the Line 39: term "white" box, as opposed to "black" box).
so it is indeed integration test.
I don't think so. What does it integrate? our python code with libguestfs? I wouldn't call this an integration test. The same logic would lead you to say that using ``print" is an integration test with libc. Line 40:
Yoav Kleinberger has posted comments on this change.
Change subject: tests: introducing whitebox tests ......................................................................
Patch Set 5: Verified+1
oVirt Jenkins CI Server has posted comments on this change.
Change subject: tests: introducing whitebox tests ......................................................................
Patch Set 5:
Build Successful
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/10479/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/11264/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/11421/ : SUCCESS
Nir Soffer has posted comments on this change.
Change subject: tests: introducing whitebox tests ......................................................................
Patch Set 4:
(1 comment)
http://gerrit.ovirt.org/#/c/29745/4/tests/README File tests/README:
Line 35: i.e. they actually open sockets, read files, write to storage, etc. This Line 36: contrasts with a unit test, in which external components should be mocked. It Line 37: also contrasts with the functional tests, in that the scope of code checked is Line 38: much smaller, and the test is aware of the underlying implementation (hence the Line 39: term "white" box, as opposed to "black" box).
I don't think so. What does it integrate? our python code with libguestfs?
Whitebox test is a test poking into the internals, compared with blackbox test that use only public intefaces and does not know anything about the implementation.
The alignment test is not a whitebox test - it is a blackbox test. You test a module using its public interface. The module happen to use libguestfs but we don't really care about this.
If you mock libguestfs, this would be a whitebox test, since you assume the usage of libguestfs.
The concept of whitebox/blackbox is not very interesting and not the main reason why this test should not be in the regular test suit. Line 40:
Adam Litke has posted comments on this change.
Change subject: tests: introducing whitebox tests ......................................................................
Patch Set 5:
(1 comment)
http://gerrit.ovirt.org/#/c/29745/5//COMMIT_MSG Commit Message:
Line 18: because they are aware of the inside implementation (as opposed to Line 19: blackbox tests), but they check that things "really work" (e.g. Line 20: libraries get called, sockets are opened, etc.) , and not just Line 21: the internal logic of some small section of code (which would Line 22: characterize a unit test). I agree with another commenter that blackbox vs. whitebox isn't interesting enough in its own right to justify all of the arguing we'd do in the future when classifying tests. I agree that the problem you are trying to solve is valid but I would frame it differently.
To me, there are some functional tests which are easily automated and can produce a valid result on all supported configurations of the software. To me these are functional tests.
We need another category for tests which require a special environment and/or are not easily automated. We should always prefer a test to be 'functional' rather than 'special' since these special tests will not be run nearly as often. However it is better to have 'special' tests than no test at all. Line 23: Line 24: Change-Id: I4e20d17c3ebee1203bb5a721ce44d5867570ce8e
Yoav Kleinberger has posted comments on this change.
Change subject: tests: introducing whitebox tests ......................................................................
Patch Set 5:
(1 comment)
http://gerrit.ovirt.org/#/c/29745/5//COMMIT_MSG Commit Message:
Line 18: because they are aware of the inside implementation (as opposed to Line 19: blackbox tests), but they check that things "really work" (e.g. Line 20: libraries get called, sockets are opened, etc.) , and not just Line 21: the internal logic of some small section of code (which would Line 22: characterize a unit test).
I agree with another commenter that blackbox vs. whitebox isn't interesting
they main reason we don't like running this test is that it's slow.
so, how to call the new category? perhaps just "slowtests". Line 23: Line 24: Change-Id: I4e20d17c3ebee1203bb5a721ce44d5867570ce8e
Nir Soffer has posted comments on this change.
Change subject: tests: introducing whitebox tests ......................................................................
Patch Set 5:
(1 comment)
http://gerrit.ovirt.org/#/c/29745/5//COMMIT_MSG Commit Message:
Line 18: because they are aware of the inside implementation (as opposed to Line 19: blackbox tests), but they check that things "really work" (e.g. Line 20: libraries get called, sockets are opened, etc.) , and not just Line 21: the internal logic of some small section of code (which would Line 22: characterize a unit test).
they main reason we don't like running this test is that it's slow.
For slow tests we have the @slowtest decorator, but using it is not very easy (make check NOSE_SKIP_SLOW_TESTS=1).
The issue is is not only the slowness but fact that this test both our code calling libguestfs, and libguestfs itself. So this may break even if our code did not change.
There is not reason to run this test each time you build vdsm, it it relevant only when libguestfs or one of its dependencies changed, or if you modified this code.
So we can call this "small" functional tests. Line 23: Line 24: Change-Id: I4e20d17c3ebee1203bb5a721ce44d5867570ce8e
Vered Volansky has posted comments on this change.
Change subject: tests: introducing whitebox tests ......................................................................
Patch Set 5: Code-Review-1
Allon Mureinik has abandoned this change.
Change subject: tests: introducing whitebox tests ......................................................................
Abandoned
outdated, feel free to reopen if you want to revive it.
automation@ovirt.org has posted comments on this change.
Change subject: tests: introducing whitebox tests ......................................................................
Patch Set 5:
* Update tracker::IGNORE, no Bug-Url found
vdsm-patches@lists.fedorahosted.org