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

Add check to see if we really are in node even if document exists #40

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

amadsen
Copy link

@amadsen amadsen commented Jun 19, 2017

This fixes an issue where document has been mocked in a node environment, but many other interfaces provided by a browser have not that causes tap-mocha-reporter to throw a RefferenceError instead of properly reporting an error documented in the TAP stream. It also adds a couple of tests and a few files to support those tests.

@amadsen
Copy link
Author

amadsen commented Jun 19, 2017

@isaacs Please let me know if there is anything I can do to improve this pull request. Thanks!

lib/utils.js Outdated
@@ -643,7 +643,7 @@ exports.getError = function(err) {

exports.stackTraceFilter = function() {
var slash = '/'
, is = typeof document === 'undefined'
, is = (typeof document === 'undefined' || /(node|iojs)/.test(((process || {}).release || {}).name))
Copy link
Member

Choose a reason for hiding this comment

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

This will throw if process is not defined.

Copy link
Author

Choose a reason for hiding this comment

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

Indeed. I have updated the pull request to use a helper function (rather than nest a ternary).
A simpler alternative would be to test for process the way the existing code tests for document and reverse the ternary - the assumption being that a browser environment is much less likely to mock the process object than a node environment is to mock document.

Also, I'll see if I can come up with a useful way to test this in a browser environment. Can you describe (or point to an example of) what that might look like?

@amadsen
Copy link
Author

amadsen commented Jun 20, 2017

So, multiple additional thoughts:

  1. the code in lib/util.js actually seems to be concerned with whether the stack trace came from a browser or node - because it is cleaning out lines that are probably not relevant to the project under test. Testing whether we are currently running in a browser or node seems like a poor approximation of that.
  2. There are (roughly) 58 uses of process throughout the code base - in many cases process.stdout is used to write output.
  3. I am fiddling around with running a 'in browser' test via browserify and browser-run, but the idea I expressed last night (browsers mocking process less often than node mocking document) is clearly wrong when I consider bundlers.
  4. The only reason the code goes to the effort to determine if it is in a browser is to conditionally remove lines referencing modules with components in the line
// node_modules, bower, componentJS
  function isBrowserModule(line) {
    return (~line.indexOf('node_modules')) ||
      (~line.indexOf('components'));
  }

versus

  function isNodeModule (line) {
    return (~line.indexOf('node_modules'));
  }

Just checking for components in the line may actually fail in a couple of ways:

  1. projects (particularly React projects like this one) often use components in a path name these days (although these are admittedly preprocessed and bundled, so... maybe not a concern)
  2. bower allows you to configure the name of your components directory, so it may not be bower_components or contain components at all.

Removing node_modules, node internals, and mocha internals(?) seems fairly safe
So, I guess the question is do we actually want to remove lines containing components given the false positives, false negatives, and variance between the source of the TAP data and the environment the reporter is running in? Not doing so would seem to allow us to eliminate the browser vs. node check altogether.

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.

2 participants