Add some testing infrastructure for feature testing examples #299

Ryman · 2015-11-21T04:21:55Z

I'd like to make an attempt soon to refactor some of the internals so that we can actually do decent unit-testing, and to remove some gotchas. To do so with any confidence we need to have some end-to-end tests in place to make sure nothing major breaks.

I've made a start here but there are many examples still not covered, most notably the large example.rs/macro_example.rs. We should split that example into several more focused examples with better documentation as I can only imagine a new user is unable to really understand what features are available when the entire sink is thrown at them. EDIT: example.rs has been split, macro_example.rs is left for future

Feedback and suggestions are extremely welcome, this feels pretty hacky, but it's the most effective way I could think of getting a decent baseline without potentially introducing errors (by making internal code easier to test).

Ryman · 2015-11-21T04:28:45Z

src/nickel.rs

@@ -156,7 +157,15 @@ impl<D: Sync + Send + 'static> Nickel<D> {
        });

        let server = Server::new(self.middleware_stack, self.data);
-        let listener = server.serve(addr, self.keep_alive_timeout).unwrap();
+
+        let is_test_harness = env::vars().any(|(ref k, _)| k == "NICKEL_TEST_HARNESS");


This is probably the part with the biggest smell.

Alternatives:

change all the examples to run on a random port (which is what I did in similar tests for nickel-mustache). The downside of that approach is that users who copy the examples have to actually pay attention to the port (and it can change between server reboots).

use something more standard such as $PORT if it exists, which might be beneficial for users who want to deploy on something like Heroku (I certainly have a tendency to always forget about setting this)

update examples to read their port from stdin. This is potentially something the user would add themselves, which would help them, but can add some verbosity to examples.

Can you clarify why the examples can't just keep running on one defined port? I'm assuming it's because it would prevent us from running multiple tests in parallel but as far as I understood it so far that won't be the case anyway because cargo doesn't work well in parallel either.

The server in the example will fail to bind to the port "6767" as it's in use, which will currently panic.

Builds are sequential, but testing (and running the build artifacts) is still in parallel. Sadly at least on my machine, likely due to multirust, a call to cargo build when it builds nothing still takes a second, so it often means things happen sequentially anyway, but occasionally I do see some tests happening in parallel. This will definitely be true for more complicated tests which may be less instantaneous.

Gotcha! I'm also for $PORT then

SimonTeixidor · 2015-11-21T14:27:33Z

This is great! Is it possible to export the util methods so that it can be used by middleware developed in a separate repo?

I'd like to see a $PORT being used if available. As you say, it should make it easier to deploy apps on different platforms.

cburgdorf · 2015-11-21T19:02:26Z

That's super super cool! I left a note regarding the port thing but it's probably just me not seeing the forest for the trees ;)

Ryman · 2015-11-21T20:29:28Z

This is great! Is it possible to export the util methods so that it can be used by middleware developed in a separate repo?

tl;dr: yes, but not yet

I'd like to extract it in future, but was hoping to keep it private and unstable until we discover the core useful interface would be. Definitely think it's worth waiting until the remaining examples are under test before continuing with that.

I'd like to see a $PORT being used if available.

Vote counted!

Ryman · 2015-11-24T18:01:17Z

So, when I sat down to implement the switch to $PORT, I realised there's a couple of problems which makes it even worse:

different semantics: NICKEL_TEST_HARNESS means do something different due to a test harness being present and can just bind to localhost, PORT on the other hand should have a port value which means we need to adjust the addresses being input.
We currently take server.listen(foo) where foo implements ToSocketAddrs. One way of overriding the port from this is to call the to_socket_addrs and update the ports for each SocketAddr in the list (assuming it parses correctly). Another would be to append a new binding to localhost:$PORT which may not be the best thing, as below. Both of these approaches seem rather hacky though.
If the aim of PORT is to make it easier to get running on heroku and such, then the default binding should probably be to 0.0.0.0 rather than localhost. On the other hand, we should not default to that for local servers.

I'm not exactly sure what the best fix is, but I'm starting to lean on NICKEL_TEST_HARNESS being OK as a private implementation detail for now.

cburgdorf · 2015-11-24T21:18:42Z

but I'm starting to lean on NICKEL_TEST_HARNESS being OK

I'm the last person to criticize for that ;)

Otherwise, cargo test will try to run each .rs file within the tests folder which will fail as many of them are intended to be submodules.

Ryman · 2015-11-25T05:43:44Z

Rebased with comment about zero port being random.

Do we just go ahead with this for now or does anybody have a neat suggestion about my last comment?

homu · 2016-01-14T21:33:55Z

⚡ Test exempted - status

Pull request: #299 Approved by: SimonPersson

Otherwise, cargo test will try to run each .rs file within the tests folder which will fail as many of them are intended to be submodules. Pull request: #299 Approved by: SimonPersson

Pull request: #299 Approved by: SimonPersson

…re examples Pull request: #299 Approved by: SimonPersson

Ryman · 2016-01-14T21:35:05Z

Well that was unexpected

cburgdorf · 2016-01-14T21:42:14Z

Erm...did homu just shake up master a bit?

cburgdorf · 2016-01-14T21:43:09Z

Ah, just a late merge it seems? So, everything is fine, ya?

Ryman · 2016-01-14T21:45:32Z

I think so, do you mean the build failure in the travis history (and resulting email)? I'm not exactly sure what causes that but it seems to not be a real issue?

cburgdorf · 2016-01-14T21:46:45Z

I just read your comment and saw this huge bunch of commits landing in master and for a moment thought homu did something wrong. Never mind me ;)

Ryman reviewed Nov 21, 2015
View reviewed changes

Ryman force-pushed the test-examples branch from b282aec to 9e32a3e Compare November 25, 2015 05:35

Ryman added 12 commits November 25, 2015 05:37

feat(server): use a random port when NICKEL_TEST_HARNESS exists in env

ff0d3e5

test(examples): basic support for testing examples

848bf7e

chore(tests): reorganise tests tree

71988af

Otherwise, cargo test will try to run each .rs file within the tests folder which will fail as many of them are intended to be submodules.

test(example): more test helpers!

deab356

test(examples): add some tests for mounting example

485260f

test(examples): enforce sequential cargo builds

fb22090

test(examples): add some tests for route_data example

2d01efa

test(examples): add some tests for default_router example

7348447

test(examples): add some tests for template example

ae17f29

test(examples): add some tests for moved_ownership example

53ea1da

test(examples): add some tests for chaining example

30caf17

fix(1.3): adjust example-tests to run on 1.3 on travis

d6592f7

Ryman force-pushed the test-examples branch from 9e32a3e to d6592f7 Compare November 25, 2015 05:40

Ryman added 5 commits November 29, 2015 05:11

refactor(examples): extract json example from example.rs

aa7eb47

test(examples): add some tests for json

b350273

refactor(examples): extract query_string example from example.rs

cd47893

test(examples): add some tests for query_string

d67ade0

refactor(examples): extract regex_route example from example.rs

812deca

Ryman mentioned this pull request Nov 30, 2015

doc(examples): add examples with custom error handling #304

Open

Ryman added 3 commits November 30, 2015 04:11

test(examples): add some tests for regex_route

9698fe3

refactor(examples): extract custom_error_handler example from example.rs

309122c

test(examples): add some tests for custom_error_handler

79e1a94

homu merged commit 22c8026 into nickel-org:master Jan 14, 2016

homu pushed a commit that referenced this pull request Jan 14, 2016

feat(server): use a random port when NICKEL_TEST_HARNESS exists in env

718d1ae

Pull request: #299 Approved by: SimonPersson

homu pushed a commit that referenced this pull request Jan 14, 2016

test(examples): basic support for testing examples

9118883

Pull request: #299 Approved by: SimonPersson

homu pushed a commit that referenced this pull request Jan 14, 2016

test(example): more test helpers!

c5ffc17

Pull request: #299 Approved by: SimonPersson

homu pushed a commit that referenced this pull request Jan 14, 2016

test(examples): add some tests for mounting example

f8faacf

Pull request: #299 Approved by: SimonPersson

homu pushed a commit that referenced this pull request Jan 14, 2016

test(examples): enforce sequential cargo builds

6c5b5ca

Pull request: #299 Approved by: SimonPersson

homu pushed a commit that referenced this pull request Jan 14, 2016

test(examples): add some tests for route_data example

047264f

Pull request: #299 Approved by: SimonPersson

homu pushed a commit that referenced this pull request Jan 14, 2016

test(examples): add some tests for default_router example

68265f9

Pull request: #299 Approved by: SimonPersson

homu pushed a commit that referenced this pull request Jan 14, 2016

test(examples): add some tests for template example

edc00be

Pull request: #299 Approved by: SimonPersson

homu pushed a commit that referenced this pull request Jan 14, 2016

test(examples): add some tests for moved_ownership example

1d679e9

Pull request: #299 Approved by: SimonPersson

homu pushed a commit that referenced this pull request Jan 14, 2016

test(examples): add some tests for chaining example

98adb00

Pull request: #299 Approved by: SimonPersson

homu pushed a commit that referenced this pull request Jan 14, 2016

fix(1.3): adjust example-tests to run on 1.3 on travis

ab0d610

Pull request: #299 Approved by: SimonPersson

homu pushed a commit that referenced this pull request Jan 14, 2016

refactor(examples): extract json example from example.rs

60f750e

Pull request: #299 Approved by: SimonPersson

homu pushed a commit that referenced this pull request Jan 14, 2016

test(examples): add some tests for json

b6f1244

Pull request: #299 Approved by: SimonPersson

homu pushed a commit that referenced this pull request Jan 14, 2016

refactor(examples): extract query_string example from example.rs

ddfd2c3

Pull request: #299 Approved by: SimonPersson

homu pushed a commit that referenced this pull request Jan 14, 2016

test(examples): add some tests for query_string

c1a4198

Pull request: #299 Approved by: SimonPersson

homu pushed a commit that referenced this pull request Jan 14, 2016

refactor(examples): extract regex_route example from example.rs

81ec6d6

Pull request: #299 Approved by: SimonPersson

homu pushed a commit that referenced this pull request Jan 14, 2016

test(examples): add some tests for regex_route

bc4f4bc

Pull request: #299 Approved by: SimonPersson

homu pushed a commit that referenced this pull request Jan 14, 2016

refactor(examples): extract custom_error_handler example from example.rs

d8b9a73

Pull request: #299 Approved by: SimonPersson

homu pushed a commit that referenced this pull request Jan 14, 2016

test(examples): add some tests for custom_error_handler

9f65a7a

Pull request: #299 Approved by: SimonPersson

homu pushed a commit that referenced this pull request Jan 14, 2016

refactor(examples): extract static_files example from example.rs

5b77e16

Pull request: #299 Approved by: SimonPersson

homu pushed a commit that referenced this pull request Jan 14, 2016

test(examples): add some tests for static_files

2e219a6

Pull request: #299 Approved by: SimonPersson

homu pushed a commit that referenced this pull request Jan 14, 2016

refactor(examples): split example.rs into routing and logger_middlewa…

9857a61

…re examples Pull request: #299 Approved by: SimonPersson

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add some testing infrastructure for feature testing examples #299

Add some testing infrastructure for feature testing examples #299

Ryman commented Nov 21, 2015

Ryman Nov 21, 2015

cburgdorf Nov 21, 2015

Ryman Nov 21, 2015

cburgdorf Nov 22, 2015

SimonTeixidor commented Nov 21, 2015

cburgdorf commented Nov 21, 2015

Ryman commented Nov 21, 2015

Ryman commented Nov 24, 2015

cburgdorf commented Nov 24, 2015

Ryman commented Nov 25, 2015

homu commented Jan 14, 2016

Ryman commented Jan 14, 2016

cburgdorf commented Jan 14, 2016

cburgdorf commented Jan 14, 2016

Ryman commented Jan 14, 2016

cburgdorf commented Jan 14, 2016

Add some testing infrastructure for feature testing examples #299

Add some testing infrastructure for feature testing examples #299

Conversation

Ryman commented Nov 21, 2015

Ryman Nov 21, 2015

Choose a reason for hiding this comment

cburgdorf Nov 21, 2015

Choose a reason for hiding this comment

Ryman Nov 21, 2015

Choose a reason for hiding this comment

cburgdorf Nov 22, 2015

Choose a reason for hiding this comment

SimonTeixidor commented Nov 21, 2015

cburgdorf commented Nov 21, 2015

Ryman commented Nov 21, 2015

Ryman commented Nov 24, 2015

cburgdorf commented Nov 24, 2015

Ryman commented Nov 25, 2015

homu commented Jan 14, 2016

Ryman commented Jan 14, 2016

cburgdorf commented Jan 14, 2016

cburgdorf commented Jan 14, 2016

Ryman commented Jan 14, 2016

cburgdorf commented Jan 14, 2016