mountebank v2.0 #9

smotes · 2019-03-31T23:37:57Z

Major

I updated the integration tests to run against a mountebank v2 instance, so we should be ready to start adding new features to the client (see ad1a6d7). However, no actual new v2 features have been added yet.
The mountebank v2 API is totally backwards compatible with v1 so I didn't have to adjust the integration tests too much. See http://www.mbtest.org/feed for more details on the backwards compatibility guarantee.

Minor/unrelated

I simplified the integration tests by quite a bit by making the assumption that a mountebank instance would already be running at localhost:2525. This allowed me to get rid of all the Docker setup code (see f8c84d6) and remove all the dep vendoring (see 05fcbd2). As a result, you'll need to install and run a local mountebank instance yourself before running the integration tests, directly via NPM or using Docker.

$ npm install --global mountebank@2.0.0
$ mb

-or-

$ docker run -d --rm --name=mountebank -p 2525:2525 -p 8080:8080 -p 8081:8081 andyrbell/mountebank:2.0.0

I refactored the internal testutil package into the assert package, to make its intent more clear (see c4e84da).
I cleaned up the Makefile a bit (see d80a1b8).

smotes · 2019-04-02T18:39:57Z

Looks like make errcheck is failing on Go v1.10 due to not supporting that version anymore.

$ make errcheck
error: failed to check packages: unsupported version of go: exit status 2: flag provided but not defined: -compiled

While I do think it's a useful tool, it's a bit of a shame to exclude testing earlier versions of Go just to maintain compatibility. Rather than remove the Go 1.10 checks from CI, I think it would be better to remove the errcheck bit. What do you think?

alcortesm · 2019-04-05T07:24:25Z

Looks like make errcheck is failing on Go v1.10 due to not supporting that version anymore.
$ make errcheck
error: failed to check packages: unsupported version of go: exit status 2: flag provided but not defined: -compiled
While I do think it's a useful tool, it's a bit of a shame to exclude testing earlier versions of Go just to maintain compatibility. Rather than remove the Go 1.10 checks from CI, I think it would be better to remove the errcheck bit. What do you think?

I think having these checks in your code is a good thing, I use similar checks myself and I find them very helpful. I would not remove them and I don't think you have to sacrifice them to have Go 1.10 compatibility.

The problem here is to attach those checks to particular Go versions. The results of these checks should not change depending on the Go version you are using, by running the checks for several Go versions you are not making your code better, just testing how well supported are those Go versions by the check tools.

I believe these checks should only be run once, in an ad-hoc environment where an specific version of Go and the check tools are pre-installed. You can do this by building a nice docker image with a recent Go version and all testing tools installed. The unit tests on the other hand can be run on Travis for a wide range of Go versions if you want, as that can help people using old versions of Go.

I think that the best way to solve this is to follow this steps:

keep Travis running unit and integration tests on different go versions, that is okay
stop Travis from running the checks (errcheck...) for every Go version
make Travis run the checks only once in a specific environment with recent versions of the tools installed. Also allow developers to do that in the makefile directly (remove the targets from the makefile that install things in developers computers, that is quite harmful if they needed specific versions of those tools for other projects)

alcortesm · 2019-04-05T07:29:27Z

Gopkg.toml

@@ -1,25 +0,0 @@
-# Gopkg.toml example


It is great that you have get rid of the dependencies.

It would be nice if you make this repo a go module, so people can start depending on it using go mod.

alcortesm · 2019-04-05T07:39:22Z

Makefile

@@ -1,29 +1,25 @@
-GOPACKAGES := $(shell go list github.com/senseyeio/mbgo/...)
+GOPACKAGES := $(shell go list ./...)



In addition to my comment above about removing all these checks from here and making it run in a docker, I think this makefile needs some changes:

there is no default target, so when you run make it runs make errcheck which is probably not what people wants

the integration tests should probably run whatever is required for it to work. If I run it manually without a mountebank instance running, the tests still pass, which is weird as the README explains they rely on some mountebank running.

I've added a default target to the Makefile which runs the fmt, unit and lint targets -- see commit 6633b7c.

I've updated the Makefile to run a new shell script to handle the integration tests, which include spinning up a Docker container for mountebank, running the integration tests, then finally removing the container -- see commit 335c505.

It's weird that the tests were passing for you even though an instance wasn't running -- I wasn't able to reproduce that locally. Maybe it was caching?

I'm sorry, I don't know what happened there, I cannot reproduce it either, maybe I didn't notice I had a mountebank instance running.

alcortesm · 2019-04-05T07:45:04Z

README.md

@@ -19,7 +19,9 @@ $ make unit
 $ make integration
 ```

-The integration test client points to a local Docker container at port 2525, with the additional ports 8080-8083 exposed for communication with test imposters. Currently tested against a mountebank v1.16.0 instance.
+The integration test client expects a version of mountebank running at `localhost:2525`, with the additional ports 8080-8081 exposed for test imposters.


Expecting users to have specific software running at specific ports is asking too much IMHO. Maybe running the integration tests in an specific environment build just for that would be a nice idea (docker maybe) to avoid people form having to setup their environment the way the lib wants.

See my comment in the Makefile diff and/or commit 335c505.

alcortesm · 2019-04-05T07:47:46Z

internal/assert/assert.go

 // tests to assert deep equality between an actual and expected value.
-func ExpectEqual(tb testing.TB, actual, expected interface{}) {
+func Equals(tb testing.TB, actual, expected interface{}) {
 	if !reflect.DeepEqual(actual, expected) {
 		_, file, line, _ := runtime.Caller(1)


You might want to use of tb.Helper instead.

Fixed in commit 378d3be.

alcortesm · 2019-04-05T07:48:37Z

internal/assert/assert.go

 // tests to assert deep equality between an actual and expected value.
-func ExpectEqual(tb testing.TB, actual, expected interface{}) {
+func Equals(tb testing.TB, actual, expected interface{}) {
 	if !reflect.DeepEqual(actual, expected) {
 		_, file, line, _ := runtime.Caller(1)
 		fmt.Printf("\033[31m%s:%d:\n\n\texp: %#v\n\n\tgot: %#v\033[39m\n\n", filepath.Base(file), line, expected, actual)


Colors don't work well across different OS.

Fixed in commit 378d3be.

…failure output

…sion

…ed source code analysis tools

…essary work

…ia docker

smotes · 2019-04-05T18:18:01Z

Looks like make errcheck is failing on Go v1.10 due to not supporting that version anymore.
$ make errcheck
error: failed to check packages: unsupported version of go: exit status 2: flag provided but not defined: -compiled
While I do think it's a useful tool, it's a bit of a shame to exclude testing earlier versions of Go just to maintain compatibility. Rather than remove the Go 1.10 checks from CI, I think it would be better to remove the errcheck bit. What do you think?
I think having these checks in your code is a good thing, I use similar checks myself and I find them very helpful. I would not remove them and I don't think you have to sacrifice them to have Go 1.10 compatibility.

The problem here is to attach those checks to particular Go versions. The results of these checks should not change depending on the Go version you are using, by running the checks for several Go versions you are not making your code better, just testing how well supported are those Go versions by the check tools.

I believe these checks should only be run once, in an ad-hoc environment where an specific version of Go and the check tools are pre-installed. You can do this by building a nice docker image with a recent Go version and all testing tools installed. The unit tests on the other hand can be run on Travis for a wide range of Go versions if you want, as that can help people using old versions of Go.

I think that the best way to solve this is to follow this steps:
1. keep Travis running unit and integration tests on different go versions, that is okay

2. stop Travis from running the checks (errcheck...) for every Go version

3. make Travis run the checks only once in a specific environment with recent versions of the tools installed. Also allow developers to do that in the makefile directly (remove the targets from the makefile that install things in developers computers, that is quite harmful if they needed specific versions of those tools for other projects)

I re-organized the Travis CI file into separate build stages, where the source code analysis stage only uses Go 1.12, whereas the tests will run against all versions. See commit b914ea7.

alcortesm · 2019-04-10T07:11:53Z

I think your last changes solve all the problems.

I believe travis builds are failing due to golint not being available in the travis environment.

smotes added 6 commits March 31, 2019 18:03

remove dep config and vendoring

05fcbd2

refactor testutil into assert package

92531bf

simplified integration test setup

f8c84d6

updated assert package usage

c4e84da

simplify makefile

d80a1b8

adjust integration tests against mountebank v2 instance

ad1a6d7

alcortesm suggested changes Apr 5, 2019

View reviewed changes

smotes changed the title ~~Smotes/mountebank v2.0~~ mountebank v2.0 Apr 5, 2019

smotes added 7 commits April 5, 2019 12:04

update assert.Equals to be a test helper, remove colors in assertion …

378d3be

…failure output

add default target to Makefile

6633b7c

update travis to run source code analysis tools only on latest go ver…

b914ea7

…sion

removed tools target in Makefile, updated travis file to install need…

cfc15dd

…ed source code analysis tools

move before_install steps into their respective stages to avoid unnec…

8251ecc

…essary work

updated integration target in Makefile to run mountebank dependency v…

335c505

…ia docker

fix scripts in travis build stages

535ea8f

trying to fix build errors

6f7b1aa

alcortesm approved these changes Apr 10, 2019

View reviewed changes

smotes added 3 commits April 12, 2019 22:53

bring test job up to top level

cd8b5ff

run integration script using shell

64b6c13

added go module support

67b72dc

smotes merged commit a41c6eb into master Apr 13, 2019

smotes deleted the smotes/mountebank-v2.0 branch July 15, 2019 18:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mountebank v2.0 #9

mountebank v2.0 #9

smotes commented Mar 31, 2019 •

edited

smotes commented Apr 2, 2019 •

edited

alcortesm commented Apr 5, 2019

alcortesm Apr 5, 2019

alcortesm Apr 5, 2019

smotes Apr 5, 2019 •

edited

alcortesm Apr 10, 2019

alcortesm Apr 5, 2019

smotes Apr 5, 2019

alcortesm Apr 5, 2019

smotes Apr 5, 2019

alcortesm Apr 5, 2019

smotes Apr 5, 2019

smotes commented Apr 5, 2019

alcortesm commented Apr 10, 2019

		@@ -1,29 +1,25 @@
		GOPACKAGES := $(shell go list github.com/senseyeio/mbgo/...)
		GOPACKAGES := $(shell go list ./...)

mountebank v2.0 #9

mountebank v2.0 #9

Conversation

smotes commented Mar 31, 2019 • edited

Major

Minor/unrelated

smotes commented Apr 2, 2019 • edited

alcortesm commented Apr 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smotes Apr 5, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smotes commented Apr 5, 2019

alcortesm commented Apr 10, 2019

smotes commented Mar 31, 2019 •

edited

smotes commented Apr 2, 2019 •

edited

smotes Apr 5, 2019 •

edited