Add support for mocking services by adamrbennett · Pull Request #90 · BlueOakJS/blueoak-server

adamrbennett · 2017-08-22T13:56:14Z

Create mock services in the test/bos-mocks/services directory and mock middleware in the test/bos-mocks/middleware directory. The mock file name must match the service or middleware you wish to mock.
Pass comma delimited service names you wish to mock in the --mock-services command line argument and pass comma delimited middleware names you wish to mock in the --mock-middleware command line argument.

seanpk

I really like this initiative.
I've requested some changes, but am open to discussion on those each point. Hopefully, however, they all make sense.

seanpk · 2017-08-22T14:11:55Z

bin/blueoak-server.js

 var server = require('../');

+// parse arguments
+var argv = parseArgs(process.argv.slice(2));


does this work if invoked as blueoak-server?

You mean like this?
blueoak-server --mocks mock-service

Instead of like this?
./node_modules/blueoak-server/bin/blueoak-server.js --mocks mock-service

Yes, it works on Node v6.8.1

seanpk · 2017-08-22T14:14:02Z

bin/blueoak-server.js

 server.init({
-    appDir: process.cwd()
+    appDir: process.cwd(),
+    mocks: mocks


maybe we should have a different script/invocation that supports mocks instead of adding testing support directly into the main command ...
say bin/bos-mock.js

Yeah, I know what you mean. My initial preference was to avoid modifying the main script, but I figured it doesn't add much overhead, and it might be nice to parse other CLI args in the future.

I took this approach for the simplicity and it lends itself well to Dockerization. With ENTRYPOINT ["blueoak-server"] arguments can be passed to the container at the command line more simply. For example, docker run blueoak-server --mocks mock-service instead of docker run --entrypoint bin/bos-mock.js blueoak-server --mocks mock-service

If we're considering this as a foundation for future CLI, should we integrate a framework like commander instead?
(I think probably...)

Yep, commander provides a better UX with usage output.

seanpk · 2017-08-22T14:19:02Z

test/integration/fixtures/server12/mocks/dummyservice.js

+ */
+exports.init = function(logger) {
+    logger.info('Dummy Service Mock initialized');
+    process.exit(0); // exit test here


I think it would be a better test for the dummy service not to exit, but to provide a function that can be called from the test after server startup has completed

Yeah that sounds much better. I might run into challenges but I'll do some exploration.

seanpk · 2017-08-22T14:21:43Z

bin/blueoak-server.js

 server.init({
-    appDir: process.cwd()
+    appDir: process.cwd(),
+    mocks: mocks


since mocks are used for testing/prototyping, I think it would make sense that their default directory is tests/mocks, or tests/bos-mocks/, or even tests/bos-mocks/services if it would ever make sense to have mocks for middleware or handlers (which it might)

Totally agree. Good point about mocking the other things. Only concern would be potential collisions with existing projects that already have test/s directories. While I prefer tests/mocks, I think tests/bos-mocks is far less likely to collide with code in existing projects.

yeah ... and it looks like test/bos-mocks is the way to go since the template project has a test dir, not a tests dir

which makes me think:

should/can the mocks directory be configurable through?

can/should the list of mocks be provided through configuration?

I could add a config to define the location of mocks, but I initially chose not to because BOS appears to favor convention over configuration for the locations of other things like handlers, services, etc.

I avoided declaring mocks in the configuration for the following reasons:

To keep the config under test as similar as possible to the release/runtime config.

To be able to easily control which services are mocked without changing config (i.e.: mocks won't be loaded simply because they exist or are declared, but only if they're also identified in the CLI argument). Although, this approach could still apply if mocks are declared in config.

Let me know how you'd like to proceed here and I'll make the necessary updates.

adamrbennett · 2017-08-22T16:12:48Z

Oops, pushed my changes before seeing your latest comments. Will address and update.

seanpk · 2017-08-25T11:49:43Z

bin/blueoak-server.js

 server.init({
    appDir: process.cwd(),
-    mocks: mocks
+    mockServices: cli.mockServices


would:

mocks: { services: cli.mockServices }

work?

Then it can be expanded to support middleware in the future

Yep, I like this better.

seanpk · 2017-08-25T11:52:06Z

lib/loader.js

        //strip the prefix off since that's not part of the module name
        parentId = parentId.indexOf('.') > -1 ? parentId.substring(parentId.indexOf('.') + 1) : parentId;
-
+        


whitespace (I can't recall if this project has the requirement to strip trailing whitespace ... I may just be used to seeing that in other code)

My editor added that, but lint passes so I guess it's ok?

seanpk · 2017-08-25T11:57:38Z

lib/subRequire.js

+        _.includes(global.__mockServices, path.basename(id, path.extname(id)))
+    ) {
+        var mockPath = path.resolve(global.__appDir, 'test', 'bos-mocks', 'services', path.basename(id));
+        if (mockPath) {


on failure to find the service, would a message be helpful?
maybe like:

logger.warn(util.format('the requested mock service "%s" was not found in the mock service directory (%s)', mockDir, mockId));

(assuming the mock directory has been calculated once into a variable named mockDir and mockId = path.basename(id) was done too ... util.format should be used for safety since users can configure different loggers)

Good idea, definitely helpful.

seanpk · 2017-08-25T12:00:13Z

test/integration/testLoader.js

-        util.launch('server6', function(output) {
-            assert.ok(output.indexOf('already exists') > -1);
-            done();
+// describe('SERVER6 - duplicate service name should fail on startup', function () {


was this exclusion meant to be temporary?

adamrbennett · 2017-08-25T21:45:37Z

I just went ahead and added support for mocking middleware to expose and address all the gremlins now. We're not waiting on this PR anymore on dependent projects, so no rush to merge -- let's get it right.

adamrbennett · 2017-08-25T22:07:15Z

lib/loader.js

                var mod;
                try {
-                    mod = require(modPath);
+                    mod = subRequire(modPath);


Middleware mocks were not loading because middleware was not loaded with subRequire. I did this to make it work, but admittedly do not understand all of the potential ramifications of this change.

seanpk · 2018-02-20T22:44:41Z

@adamrbennett - what are your thoughts on picking up this work again and finishing it off?
Or should we just close the PR?

Adam Bennett added 4 commits August 22, 2017 07:48

Add mocks command line argument

b419565

Load mock services when appropriate

bba06c9

Add test for mocks

e254f78

Use lodash to support older JS runtimes

2a35017

seanpk requested changes Aug 22, 2017

View reviewed changes

Adam Bennett added 2 commits August 22, 2017 10:08

Prepare to support mocking things other than services

ec59917

Improve mock tests

be88f40

Adam Bennett added 6 commits August 22, 2017 11:04

Use commander instead of minimist for cli option parsing

6f25a0a

Use test directory instead of tests

b7b815d

Add documentation for mocking services

4e42609

Improve and add mock tests

cf7cd71

Refactor to add support for mocking service modules

73d4235

Fix lint errors

822be83

seanpk reviewed Aug 25, 2017

View reviewed changes

Adam Bennett added 2 commits August 25, 2017 07:49

Restore tests

7b00fe0

Add support for mocking middleware

d7379f5

adamrbennett commented Aug 25, 2017

View reviewed changes

Update docs

2660153

		//strip the prefix off since that's not part of the module name
		parentId = parentId.indexOf('.') > -1 ? parentId.substring(parentId.indexOf('.') + 1) : parentId;

Conversation

adamrbennett commented Aug 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seanpk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamrbennett Aug 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamrbennett commented Aug 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamrbennett commented Aug 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seanpk commented Feb 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

adamrbennett commented Aug 22, 2017 •

edited

Loading

adamrbennett Aug 22, 2017 •

edited

Loading

adamrbennett commented Aug 25, 2017 •

edited

Loading