Merge lp:~frankban/charms/trusty/redis/tsting-fixes into lp:~juju-gui/charms/trusty/redis/trunk

[Francesco Banconi]

Makefile fixes for bundletester.

- The "local" env is no longer used by default when
  JUJU_ENV is not specified. Now the current env is
  used instead.

- The environment is no longer destroyed when ftests
  complete.

- Do not exit if bootstrap returns an error: assume
  the environment to be already bootstrapped.

- Removed the setup phony target to work around a
  bundletester bug.

For more info, see review comments at
https://bugs.launchpad.net/charms/+bug/1459345

https://codereview.appspot.com/248930043/

[Francesco Banconi (frankban)]

Reviewers: mp+262062_code.launchpad.net,

Message:
Please take a look.

Description:
Makefile fixes for bundletester.

- The "local" env is no longer used by default when
   JUJU_ENV is not specified. Now the current env is
   used instead.

- The environment is no longer destroyed when ftests
   complete.

- Do not exit if bootstrap returns an error: assume
   the environment to be already bootstrapped.

- Removed the setup phony target to work around a
   bundletester bug.

For more info, see review comments at
https://bugs.launchpad.net/charms/+bug/1459345

https://code.launchpad.net/~frankban/charms/trusty/redis/tsting-fixes/+merge/262062

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/248930043/

Affected files (+19, -13 lines):
   M Makefile
   A [revision details]

Index: Makefile
=== modified file 'Makefile'
--- Makefile 2015-06-05 09:15:51 +0000
+++ Makefile 2015-06-16 09:51:39 +0000
@@ -7,7 +7,6 @@
  # Override those values to run functional tests with a different
environment
  # or Juju series.
  SERIES=trusty
-JUJU_ENV=local

  # Define system Debian dependencies: run "make sysdeps" to install them.
  # Please keep them in alphabetical order.
@@ -21,7 +20,7 @@
  PIP = $(VENV)/bin/pip
  FTESTS=$(shell find -L tests -type f -executable | sort)

-.DEFAULT_GOAL := setup
+.DEFAULT_GOAL := $(VENV_ACTIVATE)

  .PHONY: help
  help:
@@ -38,9 +37,10 @@
   @echo 'with the SERIES environment variable and the service name with '
   @echo 'the JUJU_SERVICE_NAME environment variable, for instance:'
   @echo ' make deploy SERIES=precise JUJU_SERVICE_NAME=myredis'
-
-.PHONY: setup
-setup: $(VENV_ACTIVATE)
+ @echo -e '\nWhen using "make deploy" or "make test" or "make ftest" it '
+ @echo 'is possible to override the Juju environement that will be used '
+ @echo 'with the JUJU_ENV environment variable, for instance:'
+ @echo ' make test JUJU_ENV=ec2'

  .PHONY: sysdeps
  sysdeps:
@@ -53,7 +53,7 @@
   @touch $(VENV_ACTIVATE)

  .PHONY: bundletester
-bundletester: setup
+bundletester: $(VENV_ACTIVATE)
   $(PIP) install bundletester
   $(VENV)/bin/bundletester -l DEBUG

@@ -67,13 +67,14 @@
   juju charm proof

  .PHONY: lint
-lint: setup
+lint: $(VENV_ACTIVATE)
   @$(VENV)/bin/flake8 --show-source --exclude=$(VENV) \
    --filename *.py,install,generic-hook \
    hooks/ tests/ unit_tests/

  .PHONY: deploy
  deploy:
+ $(eval JENV := $(shell JUJU_ENV=$(JUJU_ENV) juju switch))
   @# The use of readlink below is required for OS X.
   @$(eval export JUJU_REPOSITORY:=$(shell mktemp -d `readlink -f
/tmp`/temp.XXXX))
   @echo "JUJU_REPOSITORY is $(JUJU_REPOSITORY)"
@@ -82,24 +83,27 @@
   @rsync -a . $(JUJU_REPOSITORY)/${SERIES}/$(JUJU_TEST_CHARM) \
    --exclude .git --exclude .bzr --exclude tests --exclude unit_tests
   @# Deploying the charm.
- juju deploy local:${SERIES}/$(JUJU_TEST_CHARM) $(JUJU_SERVICE_NAME)
+ juju deploy -e $(JENV) local:${SERIES}/$(JUJU_TEST_CHARM)
$(JUJU_SERVICE_NAME)

  .PHONY: test
  test: unittest ftest

  .PHONY: ftest
-ftest: setup
- juju bootstrap -e $(JUJU_ENV) --upload-tools
+ftest: $(VENV_ACTIVATE)
+ $(eval JENV := $(shell JUJU_ENV=$(JUJU_ENV) juju switch))
+ @j...

[Martin Hilton (martin-hilton)]

LGTM

https://codereview.appspot.com/248930043/

[Brad Crittenden (bac)]

LGTM with typo fixes and possible refactoring. No need for re-review.

review: Approve (code)

[Francesco Banconi (frankban)]

Thank you both for the reviews!

[lp:~frankban/charms/trusty/redis/tsting-fixes]

6. By Francesco Banconi on 2015-06-16

Changes as per review.

[Francesco Banconi (frankban)]

Please take a look.

https://codereview.appspot.com/248930043/

[Francesco Banconi (frankban)]

*** Submitted:

Makefile fixes for bundletester.

- The "local" env is no longer used by default when
   JUJU_ENV is not specified. Now the current env is
   used instead.

- The environment is no longer destroyed when ftests
   complete.

- Do not exit if bootstrap returns an error: assume
   the environment to be already bootstrapped.

- Removed the setup phony target to work around a
   bundletester bug.

For more info, see review comments at
https://bugs.launchpad.net/charms/+bug/1459345

R=martin.hilton
CC=
https://codereview.appspot.com/248930043

https://codereview.appspot.com/248930043/

=== modified file 'Makefile'
--- Makefile	2015-06-05 09:15:51 +0000
+++ Makefile	2015-06-16 12:50:54 +0000
@@ -7,7 +7,6 @@
 # Override those values to run functional tests with a different environment
 # or Juju series.
 SERIES=trusty
-JUJU_ENV=local
 
 # Define system Debian dependencies: run "make sysdeps" to install them.
 # Please keep them in alphabetical order.
@@ -21,7 +20,7 @@
 PIP = $(VENV)/bin/pip
 FTESTS=$(shell find -L tests -type f -executable | sort)
 
-.DEFAULT_GOAL := setup
+.DEFAULT_GOAL := $(VENV_ACTIVATE)
 
 .PHONY: help
 help:
@@ -38,9 +37,10 @@
 	@echo 'with the SERIES environment variable and the service name with '
 	@echo 'the JUJU_SERVICE_NAME environment variable, for instance:'
 	@echo '  make deploy SERIES=precise JUJU_SERVICE_NAME=myredis'
-
-.PHONY: setup
-setup: $(VENV_ACTIVATE)
+	@echo -e '\nWhen using "make deploy" or "make test" or "make ftest" it '
+	@echo 'is possible to override the Juju environment that will be used '
+	@echo 'with the JUJU_ENV environment variable, for instance:'
+	@echo '  make test JUJU_ENV=ec2'
 
 .PHONY: sysdeps
 sysdeps:
@@ -53,7 +53,7 @@
 	@touch $(VENV_ACTIVATE)
 
 .PHONY: bundletester
-bundletester: setup
+bundletester: $(VENV_ACTIVATE)
 	$(PIP) install bundletester
 	$(VENV)/bin/bundletester -l DEBUG
 
@@ -67,13 +67,17 @@
 	juju charm proof
 
 .PHONY: lint
-lint: setup
+lint: $(VENV_ACTIVATE)
 	@$(VENV)/bin/flake8 --show-source --exclude=$(VENV) \
 		--filename *.py,install,generic-hook \
 		hooks/ tests/ unit_tests/
 
+.PHONY: jenv
+jenv:
+	$(eval JENV := $(shell JUJU_ENV=$(JUJU_ENV) juju switch))
+
 .PHONY: deploy
-deploy:
+deploy: jenv
 	@# The use of readlink below is required for OS X.
 	@$(eval export JUJU_REPOSITORY:=$(shell mktemp -d `readlink -f /tmp`/temp.XXXX))
 	@echo "JUJU_REPOSITORY is $(JUJU_REPOSITORY)"
@@ -82,24 +86,26 @@
 	@rsync -a . $(JUJU_REPOSITORY)/${SERIES}/$(JUJU_TEST_CHARM) \
 		--exclude .git --exclude .bzr --exclude tests --exclude unit_tests
 	@# Deploying the charm.
-	juju deploy local:${SERIES}/$(JUJU_TEST_CHARM) $(JUJU_SERVICE_NAME)
+	juju deploy -e $(JENV) local:${SERIES}/$(JUJU_TEST_CHARM) $(JUJU_SERVICE_NAME)
 
 .PHONY: test
 test: unittest ftest
 
 .PHONY: ftest
-ftest: setup
-	juju bootstrap -e $(JUJU_ENV) --upload-tools
+ftest: $(VENV_ACTIVATE) jenv
+	@juju bootstrap -e $(JENV) --upload-tools || \
+		echo "reusing already bootstrapped $(JENV) environment"
 	@ # Wait for the environment to be ready.
-	juju status -e $(JUJU_ENV)
+	@juju status -e $(JENV)
 	@# Setting the path is required because internally amulet calls
 	@# juju-deployer using subprocess.
 	PATH="$(VENV)/bin:$(PATH)" $(NOSE) --verbosity 2 -s -x $(FTESTS)
-	juju destroy-environment $(JUJU_ENV) -y
 	@# Clean up deployer garbage.
 	@-$(RM) -rf trusty
+	@echo "Done! Remember to destroy the environment with:"
+	@echo "juju destroy-environment $(JENV) -y"
 
 .PHONY: unittest
-unittest: setup
+unittest: $(VENV_ACTIVATE)
 	$(NOSE) --verbosity 2 -s -w unit_tests \
 		--with-coverage --cover-package hooks --cover-erase