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

the combination of #73 and #66 broke tests on master #76

Merged
merged 1 commit into from
Dec 6, 2015
Merged

Conversation

bcoe
Copy link
Member

@bcoe bcoe commented Dec 6, 2015

combining #73 and #66 broke tests on master:

  • since Improve source maps #66 rewrites paths based on the source-map, the paths being used to lookup coverage info in tests were broken.
  • removing babel from nyc in Rewrite custom require hook & source map tests #73 identified an issue with require.main.paths() I've instead moved to an approach that: 1. attempts requiring from the current working directory 2. falls back to requiring the full path of the additional module.

CC: @Lalem001, @novemberborn

bcoe added a commit that referenced this pull request Dec 6, 2015
the combination of #73 and #66 broke tests on master
@bcoe bcoe merged commit 50ef95d into master Dec 6, 2015
@bcoe bcoe deleted the fix-master branch December 6, 2015 22:12
@@ -7,6 +7,8 @@ var fixture = require('source-map-fixtures').inline('branching')
// Coverage for the fixture is stored relative to the root directory. Here
// compute the path to the fixture file relative to the root directory.
var relpath = './' + path.relative(path.join(__dirname, '..'), fixture.file)
// the sourcemap itself remaps the path.
var mappedPath = './' + path.join(path.dirname(relpath), '../src/branching.js')
Copy link
Contributor

Choose a reason for hiding this comment

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

I'll see if I can add this to to the fixture object instead.

@novemberborn
Copy link
Contributor

@bcoe well I hadn't updated #66 yet to add tests ;-)

@novemberborn novemberborn mentioned this pull request Dec 7, 2015
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