Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add Router route method (#19) #33

Merged
merged 1 commit into from
Mar 24, 2019

Conversation

yokuze
Copy link
Contributor

@yokuze yokuze commented Mar 18, 2019

No description provided.

@jthomerson
Copy link
Member

@yokuze back to you to rebase because merging your #29 unfortunately created a merge conflict here.

@coveralls
Copy link

coveralls commented Mar 21, 2019

Coverage Status

Coverage increased (+0.03%) to 99.205% when pulling 1174414 on yokuze:add_router_route_19 into 3546be4 on silvermine:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 99.205% when pulling 2f6b64c on yokuze:add_router_route_19 into 9c33a1e on silvermine:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 99.205% when pulling 2f6b64c on yokuze:add_router_route_19 into 9c33a1e on silvermine:master.

export interface RouteProcessorAppender<T> {

/**
* @param handlers the processors to mount to this route's path
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indentation

* // Runs for all HTTP verbs
* })
* .get(function(req, res, next) {
* res.json(...);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment

assert.calledOnce(putSpy);
assert.notCalled(getSpy);
assert.notCalled(postSpy);
postSpy.resetHistory();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrong one

_.each(handlers, (handler) => { assert.calledOnce(handler); });

// Check that the "all" handler was called for each method
expect(allHandler.callCount).to.strictlyEqual(methods.length);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could refactor these checks to be inside the testOutcome each ... otherwise we're not really testing that the right handler was called on each outcome.

@jthomerson jthomerson force-pushed the add_router_route_19 branch from 2f6b64c to 1174414 Compare March 24, 2019 14:38
@jthomerson
Copy link
Member

I pushed these changes.

diff --git a/src/interfaces.ts b/src/interfaces.ts
index 86b76d1..50da097 100644
--- a/src/interfaces.ts
+++ b/src/interfaces.ts
@@ -187,6 +187,7 @@ export interface IRouter {
     *       // Runs for all HTTP verbs
     *    })
     *    .get(function(req, res, next) {
+    *       // Handle GETs to /hello
     *       res.json(...);
     *    })
     *    .post(function(req, res, next) {
@@ -247,7 +248,7 @@ export interface IRoute {
 export interface RouteProcessorAppender<T> {
 
    /**
-   * @param handlers the processors to mount to this route's path
-   */
+    * @param handlers the processors to mount to this route's path
+    */
    (...handlers: ProcessorOrProcessors[]): T;
 }
diff --git a/tests/integration-tests.test.ts b/tests/integration-tests.test.ts
index a2d806a..11efd89 100644
--- a/tests/integration-tests.test.ts
+++ b/tests/integration-tests.test.ts
@@ -623,12 +623,11 @@ describe('integration tests', () => {
          assert.calledOnce(putSpy);
          assert.notCalled(getSpy);
          assert.notCalled(postSpy);
-         postSpy.resetHistory();
+         putSpy.resetHistory();
       });
 
       it('registers route handlers properly', () => {
-         let handlers: SinonSpy[] = [],
-             methods: (keyof IRoute & keyof IRouter)[],
+         let methods: (keyof IRoute & keyof IRouter)[],
              allHandler: SinonSpy,
              route: IRoute;
 
@@ -643,34 +642,46 @@ describe('integration tests', () => {
 
 
          // Register a handler for each method
-         _.each(methods, (method) => {
-            let handler = spy((_req: Request, resp: Response) => { resp.send('Test'); });
+         const handlers = _.reduce(methods, (memo, method) => {
+            let handler = spy((_req: Request, resp: Response) => { resp.send(`Test ${method}`); });
 
             // Save the handler spy for testing later
-            handlers.push(handler);
+            memo[method] = handler;
 
+            // add the handler to our route
             route[method](handler);
-         });
+
+            return memo;
+         }, {} as { [k: string]: SinonSpy });
 
          app.use((_req: Request, resp: Response) => {
             resp.send('not found');
          });
 
          // Run once for each method type
-         _.each(methods, (method) => {
-            testOutcome(method.toUpperCase(), '/test', 'Test');
+         // Both a path with and without a trailing slash should match
+         _.each([ '/test', '/test/' ], (path) => {
+            _.each(methods, (method) => {
+               testOutcome(method.toUpperCase(), path, `Test ${method}`);
+
+               // Check that the "all" handler was called
+               assert.calledOnce(allHandler);
+               allHandler.resetHistory();
+
+               // Check that only the one handler was called
+               _.each(handlers, (handler, handlerMethod) => {
+                  if (method === handlerMethod) {
+                     assert.calledOnce(handler);
+                  } else {
+                     assert.notCalled(handler);
+                  }
+                  handler.resetHistory();
+               });
+            });
          });
 
-         // Check that each handler was called exactly once
-         _.each(handlers, (handler) => { assert.calledOnce(handler); });
-
-         // Check that the "all" handler was called for each method
-         expect(allHandler.callCount).to.strictlyEqual(methods.length);
-
          // Other tests
          _.each(methods, (method) => {
-            // Ensure trailing slash matches
-            testOutcome(method.toUpperCase(), '/test/', 'Test');
             // Ensure only exact matches trigger the route handler
             testOutcome(method.toUpperCase(), '/test/anything', 'not found');
          });

@jthomerson jthomerson merged commit 663f711 into silvermine:master Mar 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants