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

Bug 913 #914

Merged
merged 2 commits into from
Jan 5, 2019
Merged

Bug 913 #914

merged 2 commits into from
Jan 5, 2019

Conversation

AGrzes
Copy link
Contributor

@AGrzes AGrzes commented Sep 5, 2018

Proposed fix for #913.
I created simple falling test and did small change to eliminate the issue.
Please let me know if any improvement is needed.

@bcoe bcoe requested a review from coreyfarrell October 6, 2018 20:06
@bcoe
Copy link
Member

bcoe commented Oct 6, 2018

@coreyfarrell @AGrzes this seems reasonable to me. tldr; you're proposing that files that fail the shouldInstrument test should not be processed during the --all step.

One question for you, I believe sometimes typescript files are laid out in a different directory, and than compiled in another folder, e.g., src/ which gets compiled into lib/; how would this effect include rules that reference lib/ and not src/.

@bcoe bcoe added the triaged label Oct 6, 2018
@AGrzes
Copy link
Contributor Author

AGrzes commented Oct 9, 2018

My reasoning (regarding old code) id the following

  this.walkAllFiles(this.cwd, function (filename) {
    filename = path.resolve(_this.cwd, filename)
    _this.addFile(filename)
    var coverage = coverageFinder()
    var lastCoverage = _this.instrumenter().lastFileCoverage()
    if (lastCoverage) {
      filename = lastCoverage.path
    }
    if (lastCoverage && _this.exclude.shouldInstrument(filename)) {
      coverage[filename] = lastCoverage
    }
})
NYC.prototype.addFile = function (filename) {
  var relFile = path.relative(this.cwd, filename)
  var source = this._readTranspiledSource(path.resolve(this.cwd, filename))
  var instrumentedSource = this._maybeInstrumentSource(source, filename, relFile)

  return {
    instrument: !!instrumentedSource,
    relFile: relFile,
    content: instrumentedSource || source
  }
}
NYC.prototype._maybeInstrumentSource = function (code, filename, relFile) {
  var instrument = this.exclude.shouldInstrument(filename, relFile)
  if (!instrument) {
    return null
  }

  var ext, transform
  for (ext in this.transforms) {
    if (filename.toLowerCase().substr(-ext.length) === ext) {
      transform = this.transforms[ext]
      break
    }
  }

  return transform ? transform(code, {filename: filename, relFile: relFile}) : null
}
  • the addFile is transpiling source (causing the issue reported by me).
  • _maybeInstrumentSource will instrument the source based on shouldInstrument check.
  • The result of instrumentation will be stored in coverage variable based on shouldInstrument check.
  • In essence if shouldInstrument returns false then any instrumentation results are discarded so I believe my change does not change end result (in cases where there was no error during transpiling)

Regarding src and lib I'm not sure I get your question. Could you explain further?

Copy link
Member

@coreyfarrell coreyfarrell left a comment

Choose a reason for hiding this comment

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

This looks good to me.

@stale
Copy link

stale bot commented Jan 5, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Jan 5, 2019
@AGrzes
Copy link
Contributor Author

AGrzes commented Jan 5, 2019

Is there anything else to do / explain before this request can be merged?

@stale stale bot removed the wontfix label Jan 5, 2019
Copy link
Member

@JaKXz JaKXz left a comment

Choose a reason for hiding this comment

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

Sorry @AGrzes this is great! going to merge now.

@JaKXz JaKXz merged commit 40afc5f into istanbuljs:master Jan 5, 2019
@@ -440,6 +440,26 @@ describe('nyc', function () {
return done()
})

it('Do not transpiles files when not included', function (done) {
Copy link
Member

Choose a reason for hiding this comment

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

should read:

"does not transpile files when not included"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants