fix(testlab): decache files in TestSandbox when resetting it

[virkt25]

Signed-off-by: Taranveer Virk taranveer@virk.cc

Ran into an issue when working on DataSource Declarative support -- in writing the tests as follows:

• Write a file in TestSandbox -- require it -- reset sandbox
• Write a new file with same name as previous -- require it -- You will get the contents of the previous file as require caches the file.

This PR fixes that by decaching *.json and *.js files in the sandbox when the sandbox is reset.

cc: @bajtos you warned me this would happen and it came to bite me in the behind. Took me 2 hours to realize what was happening when the test kept failing.

Checklist

• npm test passes on your machine
• New tests added or existing tests modified to cover all changes
• Code conforms with the style guide
• API Documentation in code was updated
• Documentation in /docs/site was updated
• Affected artifact templates in packages/cli were updated
• Affected example projects in examples/* were updated

[bajtos]

Lovely, we should have fixed this issue long time ago 👏

I personally prefer a slightly different implementation that does not require glob and decache dependencies, see https://github.com/strongloop/loopback-workspace/blob/f3fc7f12db9501a5cd8d6d24d0979f3ce8cea90c/test/support.js#L61-L65:

  // Remove any cached modules from SANDBOX
  for (var key in require.cache) {
    if (key.slice(0, SANDBOX.length) == SANDBOX)
      delete require.cache[key];
  }

The condition can be further simplified using String.prototype.startsWith, now that we can use ES6/ES2017 in our code.

[raymondfeng]

I'm not very confident in the robustness of cleaning up require cache. Would it be better to always have a new temporary folder for the sandbox upon reset?

[virkt25]

@raymondfeng What are your concerns with cleaning up the require cache -- going with @bajtos 's proposed implementation.

TestSandbox can be created anywhere a user wants so I don't think deleting it and recreating it will solve the problem. And making it truly random by getting os.tmpdir() will require the user to always check the path.

[raymondfeng]

What are your concerns with cleaning up the require cache -- going with @bajtos 's proposed implementation.

In the past, I had trouble making decache working when I was trying to reload oracledb to test if the oracle node.js driver is functional.

TestSandbox can be created anywhere a user wants so I don't think deleting it and recreating it will solve the problem. And making it truly random by getting os.tmpdir() will require the user to always check the path.

We have two choices:

• Use os.tmpdir() if the caller does not provide a root path
• Create a random child folder if a root path is provided

The reset will internally clean up the current path such as root/uuid-1 and recreate root/uuid-2. The change of physical directory of the sandbox is transparent to its consumer.

[bajtos]

I am personally not a big fan of using a temp dir:

• the temp dir is difficult to find
• node_modules from our monorepo are not accessible from /tmp

The "random child folder" approach is something that has been working well for us in loopback-boot, see here: https://github.com/ebbnormal/loopback-boot/blob/f7108a72558b24ec27be7c320b89329a17402d62/test/helpers/appdir.js#L15-L25

appdir.init = function(cb) {
  // Node's module loader has a very aggressive caching, therefore
  // we can't reuse the same path for multiple tests
  // The code here is used to generate a random string
  require('crypto').randomBytes(5, function(err, buf) {
    if (err) return cb(err);
    var randomStr = buf.toString('hex');
    PATH = appdir.PATH = sandbox.resolve(randomStr);
    cb(null, appdir.PATH);
  });
};

Another benefit of using a random subfolder: I think it can fix some (if not all) issues you have discovered while trying out parallel-mocha, @virkt25.

Having said that, I am ok with cleaning require.cache as a short-term solution to unblock DP3 work.

[bajtos]

Should we check that the key starts with this.path + '/' to ensure we don't accidentally decache file names like sandbox-123/file.js or sandbox.bak/file.js when this.path = 'sandbox'? (Plus handle \ on Windows.)

Not a big deal, maybe fixing this edge case is not worth the implementation complexity.