Issue #5 Add minishift smoke tests #18
Conversation
|
@LalatenduMohanty Could you review PR? |
|
@budhrg I am not able to install avocado framework on OS X. Looking in to the issue. |
coolbrg
changed the title
| @@ -32,3 +34,18 @@ release: b2d_iso get_gh-release | |||
| cp $(BUILD_DIR)/minishift-b2d.iso release/ | |||
| $(BUILD_DIR)/gh-release checksums sha256 | |||
| $(BUILD_DIR)/gh-release create minishift/minishift-b2d-iso $(VERSION) master v$(VERSION) | |||
|
|
|||
| .PHONY: prepare_for_test | |||
There was a problem hiding this comment.
This is actually not a phony target. This is very much a target which you want to skip if the binary is there
| .PHONY: prepare_for_test | ||
| prepare_for_test: DOCKER_MACHINE_PATH=$(BIN_DIR)/docker-machine | ||
| prepare_for_test: | ||
| @if [ ! -x $(DOCKER_MACHINE_PATH) ]; then \ |
There was a problem hiding this comment.
This if statement is redundant, provided you make this a non phony target. Let's use the tools we use properly.
| @@ -32,3 +34,18 @@ release: b2d_iso get_gh-release | |||
| cp $(BUILD_DIR)/minishift-b2d.iso release/ | |||
| $(BUILD_DIR)/gh-release checksums sha256 | |||
| $(BUILD_DIR)/gh-release create minishift/minishift-b2d-iso $(VERSION) master v$(VERSION) | |||
|
|
|||
| .PHONY: prepare_for_test | |||
| prepare_for_test: DOCKER_MACHINE_PATH=$(BIN_DIR)/docker-machine | |||
There was a problem hiding this comment.
Why don't you just set the variable at the top of the makefile? This variable does not change depending on the target
There was a problem hiding this comment.
Also, why are we installing docker-machine? We want to add minishift smoke tests, right? So I would expect that this target gets the latest minishift release and "installs" it
There was a problem hiding this comment.
Also, the Makefile seems to hardcode a Linux install. It would be nice if the right minishift version would be downloaded, so that for example these smoke tests can be run on OS X as well. I would not make this a requirement for getting this merged, but in the very least we would need a follow up issue trying to address OS problems.
| @@ -11,6 +12,7 @@ init: | |||
| .PHONY: clean | |||
| clean: | |||
| rm -rf $(BUILD_DIR) | |||
| rm -rf $(BIN_DIR) | |||
There was a problem hiding this comment.
If you remove the bin dir as part of each clean, then I argue, you should make it a sub-directory of BUILD_DIR. Usually I am of the point of view, everything which gets installed/created during the build goes somewhere under build. A reason for not doing this, would be to have something which takes a long time to do (for example downloading the Vagrant boxes in CDK 2) and you want to cache it separately. If you want to do this, then you need two clean targets. Otherwise there is no use of this distinction. You could have for example 'clean' and 'clean-all'. It depends how you want to look at this. Right now, however, the potential purpose of "caching" the binary is not achieved.
| <a name="tests"></a> | ||
| ## Tests | ||
|
|
||
| Tests are written as Python scripts under `tests` directory and run through [Avocado](avocado-framework.readthedocs.io) framework. Ensure you have _Avocado_ installed by following [Avocado instllation guide](http://avocado-framework.readthedocs.io/en/44.0/GetStartedGuide.html#installing-avocado) before running any tests. |
There was a problem hiding this comment.
How would I install Avocado in OS X? I cannot install it via the package manager and it is not part of Mac Ports nor Homebrew (afaict)
There was a problem hiding this comment.
@hferentschik , It can be install via pip.
@amitkrout has installed in Mac OS https://paste.fedoraproject.org/525918/48421257.
There was a problem hiding this comment.
@amitkrout has installed in Mac OS
How? what are the requirements and what do I execute?
There was a problem hiding this comment.
I am confused by the paste:
dhcp-0-106:~ cdkqe$ avocado -v
Avocado 36.0lts
So at this stage Avocado seems already installed
dhcp-0-106:~ cdkqe$ pip install avocado-framework
and now avocado-framework is installed? What's that.
I don't make having OS X instructions a requirement to merge this, but at some stage I would appreciate if someone provides some re-producable instructions.
There was a problem hiding this comment.
In the paste, avocado seems to be installed at the time avocado-framework is installed.
At some stage I'd like re-producable instructions on how to do this on OS X. However, this shall not hold up a merge of this.
There was a problem hiding this comment.
Yes @hferentschik , we need that for OS X too. Will check with @amitkrout to see how he installed successfully .
|
|
||
| def test_boot_vm_out_of_iso(self): | ||
| ''' Test booting up VM out of ISO ''' | ||
| cmd = self.bin_dir + "docker-machine create %s -d kvm --kvm-boot2docker-url=%s" % (self.docker_machine_vm, self.iso_file) |
There was a problem hiding this comment.
I am confused now. I thought we are adding minishift smoke tests. This is docker-machine, right? So at best we can verify that VM creation works. Fair enough, but for this particular repo I am more interested that this works with the latest version of minishift.
Not me either. There are instructions missing. AFAIR, there were some customs steps needed for install Avocado on OS X. The QE had some info around that. @LalatenduMohanty and even if you could install Avocado it would still not work for you, since other parts (docker-machine install) is Linux specific. So right now, tests will anyways only run on Linux. I am ok with that to move things along, but it should be fairly simple to get this to work on OS X as well. IMO, this could be done in a follow up issue. As long as it builds on CI I am fine for now However, I had minishift smoke tests in mind, not docker-machine! |
coolbrg
changed the title
|
$ system_profiler SPSoftwareDataType Avocado 42 installed without any error |
|
@hferentschik @LalatenduMohanty @praveenkumar Updated the PR for minishift only tests. Also, enabled running tests with showing logs on choice. Once, we are fine with these tests, I will same for centos too. It's almost the exact copy/PR. Log formats (Unfortunately only
|
| @echo "Downloading minishift binary..." | ||
| @mkdir -p $(BIN_DIR) | ||
| @cd $(BIN_DIR) && \ | ||
| curl -LO --progress-bar https://github.com/minishift/minishift/releases/download/v1.0.0-beta.2/minishift-1.0.0-beta.2-linux-amd64.tgz && \ |
There was a problem hiding this comment.
Doesn't this smoke test now only works with linux because we are downloading only linux binary to do the testing?
Smoke test suppose to run in every OS so might be adding OS specific flag would be nice.
There was a problem hiding this comment.
I think @hferentschik already specified it so let's go right now with it and then add a follow up issue.
There was a problem hiding this comment.
@budhrg hardcoded version here doesn't look good for minishift can we have some kind of script which will always retrieve latest available tarball. since we are using avocado so we assuming python would be there and so request module.
Check out https://gist.github.com/praveenkumar/f5e26a20f2b6a6640b86bc2934371bf9 if that make sense.
| self.repo_dir = os.path.dirname(os.path.realpath(__file__)) + "/.." | ||
| self.scripts_dir = self.repo_dir + "/tests/" | ||
| self.bin_dir = self.repo_dir + "/build/bin/" | ||
| self.driver_name = 'kvm' |
There was a problem hiding this comment.
Here again it's OS specific, we can add flags to get proper driver,
There was a problem hiding this comment.
+1, there are several things which won't work on other OS types, unless they are fixed.
@budhrg what's your intention? Do you want this to work on say OS X as well right now, or was your intention to fix this just for CI. We said both is ok, as long as in the long run we can get it to work on other OSes as well.
There was a problem hiding this comment.
@budhrg If your intention is to make this work for other OS types right away, then this is indeed not going to work. Why can you not use the default (at least for now). Why setting the driver explicitly?
There was a problem hiding this comment.
So what is the reply to this? For now only CentOS and Linux, right?
| self.assertEqual(0, process.returncode) | ||
| self.assertEqual('Stopped', output.rstrip()) | ||
|
|
||
| def test_removing_vm(self): |
There was a problem hiding this comment.
instead of remove I would say let's call it test_delete_vm just to align with our methods.
|
Created the follow up issue #20 to add support for Mac and Windows OS |
| @echo "Downloading minishift binary..." | ||
| @mkdir -p $(BIN_DIR) | ||
| @cd $(BIN_DIR) && \ | ||
| curl -LO --progress-bar https://github.com/minishift/minishift/releases/download/v1.0.0-beta.2/minishift-1.0.0-beta.2-linux-amd64.tgz && \ |
There was a problem hiding this comment.
@budhrg hardcoded version here doesn't look good for minishift can we have some kind of script which will always retrieve latest available tarball. since we are using avocado so we assuming python would be there and so request module.
Check out https://gist.github.com/praveenkumar/f5e26a20f2b6a6640b86bc2934371bf9 if that make sense.
|
Updated to fetch latest minishift version while downloading it. |
|
|
||
| OS = os.getenv('OS', 'linux') | ||
|
|
||
| def main(): |
There was a problem hiding this comment.
LOL, in python. I guess the tests are as well. If it works I am good.
There was a problem hiding this comment.
@hferentschik Works like charm 😄 , provided by @praveenkumar in gist
| self.repo_dir = os.path.dirname(os.path.realpath(__file__)) + "/.." | ||
| self.scripts_dir = self.repo_dir + "/tests/" | ||
| self.bin_dir = self.repo_dir + "/build/bin/" | ||
| self.driver_name = 'kvm' |
There was a problem hiding this comment.
+1, there are several things which won't work on other OS types, unless they are fixed.
@budhrg what's your intention? Do you want this to work on say OS X as well right now, or was your intention to fix this just for CI. We said both is ok, as long as in the long run we can get it to work on other OSes as well.
| def test_boot_vm_out_of_iso(self): | ||
| ''' Test booting up VM out of ISO ''' | ||
| start_args = (self.driver_name, "file://" + self.iso_file) | ||
| cmd = self.bin_dir + "minishift start --vm-driver %s --iso-url %s" % start_args |
There was a problem hiding this comment.
Right, so now we have Minishift tests ;-)
| <a name="tests"></a> | ||
| ## Tests | ||
|
|
||
| Tests are written as Python scripts under `tests` directory and run through [Avocado](avocado-framework.readthedocs.io) framework. Ensure you have _Avocado_ installed by following [Avocado instllation guide](http://avocado-framework.readthedocs.io/en/44.0/GetStartedGuide.html#installing-avocado) before running any tests. |
There was a problem hiding this comment.
@amitkrout has installed in Mac OS
How? what are the requirements and what do I execute?
| <a name="tests"></a> | ||
| ## Tests | ||
|
|
||
| Tests are written as Python scripts under `tests` directory and run through [Avocado](avocado-framework.readthedocs.io) framework. Ensure you have _Avocado_ installed by following [Avocado instllation guide](http://avocado-framework.readthedocs.io/en/44.0/GetStartedGuide.html#installing-avocado) before running any tests. |
There was a problem hiding this comment.
I am confused by the paste:
dhcp-0-106:~ cdkqe$ avocado -v
Avocado 36.0lts
So at this stage Avocado seems already installed
dhcp-0-106:~ cdkqe$ pip install avocado-framework
and now avocado-framework is installed? What's that.
I don't make having OS X instructions a requirement to merge this, but at some stage I would appreciate if someone provides some re-producable instructions.
| <a name="tests"></a> | ||
| ## Tests | ||
|
|
||
| Tests are written as Python scripts under `tests` directory and run through [Avocado](avocado-framework.readthedocs.io) framework. Ensure you have _Avocado_ installed by following [Avocado instllation guide](http://avocado-framework.readthedocs.io/en/44.0/GetStartedGuide.html#installing-avocado) before running any tests. |
There was a problem hiding this comment.
In the paste, avocado seems to be installed at the time avocado-framework is installed.
At some stage I'd like re-producable instructions on how to do this on OS X. However, this shall not hold up a merge of this.
| self.repo_dir = os.path.dirname(os.path.realpath(__file__)) + "/.." | ||
| self.scripts_dir = self.repo_dir + "/tests/" | ||
| self.bin_dir = self.repo_dir + "/build/bin/" | ||
| self.driver_name = 'kvm' |
There was a problem hiding this comment.
@budhrg If your intention is to make this work for other OS types right away, then this is indeed not going to work. Why can you not use the default (at least for now). Why setting the driver explicitly?
| def test_boot_vm_out_of_iso(self): | ||
| ''' Test booting up VM out of ISO ''' | ||
| start_args = (self.driver_name, "file://" + self.iso_file) | ||
| cmd = self.bin_dir + "minishift start --vm-driver %s --iso-url %s" % start_args |
There was a problem hiding this comment.
+1 Right, minishift tests now :-)
|
|
||
| OS = os.getenv('OS', 'linux') | ||
|
|
||
| def main(): |
There was a problem hiding this comment.
LOL, Python. If it works ;-)
| @mkdir -p $(BIN_DIR) | ||
| @cd $(BIN_DIR) && \ | ||
| curl -LO --progress-bar $(MINISHIFT_LATEST_URL) && \ | ||
| tar xzf $(ARCHIVE_FILE) |
There was a problem hiding this comment.
Just for the record, this might work for OS X and Linux, but not Windows. Remember there we have a zip.
@hferentschik I have already created the follow up issue #20 |
|
Something seems wrong to me. Check below result |
|
@praveenkumar , It seems you missed on step here before running You need to run |
|
retest this please |
2 similar comments
|
retest this please |
|
retest this please |
|
Tested and worked as expected 👍 |
|
retest this please |
2 similar comments
|
retest this please |
|
retest this please |
|
@hferentschik Tests are passing in ci.centos and ready to merge now. Once, it is merged will put similar tests for centos-iso repo here minishift/minishift-centos-iso#80 |
There was a problem hiding this comment.
I still cannot see which targets would clean the BIN_DIR and hence minishift. It is part of the test install, so there should be a way to clean it as well.
Also, there seems to be still at least one place where tests becomes OS specific. At some stage I'd like to run these tests locally as well.
That said, I am ok to merge this to make some progress.
| self.repo_dir = os.path.dirname(os.path.realpath(__file__)) + "/.." | ||
| self.scripts_dir = self.repo_dir + "/tests/" | ||
| self.bin_dir = self.repo_dir + "/build/bin/" | ||
| self.driver_name = 'kvm' |
There was a problem hiding this comment.
So what is the reply to this? For now only CentOS and Linux, right?
Yes @hferentschik |
I have added BIN_DIR as part of BUILD_DIR which is being cleaned via |
Got you. +1 |
|
Merged, thanks |
Fix #5
Should need changes from PR #13