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

Fix SourceMap sources paths #33

Merged
merged 1 commit into from
Sep 16, 2016
Merged

Fix SourceMap sources paths #33

merged 1 commit into from
Sep 16, 2016

Conversation

cgalvarez
Copy link

Description

I decided grabbing the bull by its horns and instrumented ALL files to finally fix SourceMap sources paths. The final result is this PR. These are the most significative changes:

  • Getting the package(s) under test (when run in test-packages mode) and checking whether executing from inside/outside a package folder is done only once at startup (boot).
  • The new algorithm is heavily based on the build files auto-generated by Meteor and thus it's more accurate than previous.
  • Removed fs.existsSync() calls since it's deprecated and replaced them with fs.accessSync() instead.
  • Created some helper functions (isAccessible(), parseJSON) that refactor code of common actions and improve code readability.
  • Fixed an issue with context when calling alterSourceMapPaths() from SourceMap.registerSourceMap() which causes SourceMap properties to be unavailable sometimes (undefined).
  • Now source code is more readable and easier to understand.
  • Removed environment variables (process.env.PWD) not related to or set by Meteor.
  • Now the user is warned when COVERAGE_VERBOSE=1 and any source in a SourceMap cannot be accessed or resolved, so they can provide useful logs if any future issue.
  • The source patterns are now easily tracked in the code, including the new one found.
  • Changed type of message thrown when no source map found from error to info (non-existent source map should not be treated as an error).

Motivation and Context

This PR closes previous PR #22 and issue #21 .

The previous PR was more difficult to understand and trace.

How Has This Been Tested?

Tested with meteor test-packages ./ --driver-package practicalmeteor:mocha --settings settings.coverage.json, run inside a clone of the branch dev-1.0.0 of this package.

My development/testing environment:

  • Versions used:
    • meteor@1.4.1.1
    • lmieulet:meteor-coverage@dev-1.0.0
    • practicalmeteor:mocha@2.4.5_6
  • Environment name and version: Node.js v6.3.1
  • Operating System and version: Ubuntu Xenial Desktop 16.04

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 7c040ad on cgalvarez:dev-1.0.0-fix_source_maps into * on serut:dev-1.0.0*.

@codecov-io
Copy link

Current coverage is 74.52% (diff: 85.71%)

Merging #33 into dev-1.0.0 will decrease coverage by 25.47%

@@           dev-1.0.0        #33   diff @@
===========================================
  Files              1         22    +21   
  Lines             43        793   +750   
  Methods            0          0          
  Messages           0          0          
  Branches           0          0          
===========================================
+ Hits              43        591   +548   
- Misses             0        202   +202   
  Partials           0          0          

Powered by Codecov. Last update db397ad...7c040ad

@serut
Copy link
Owner

serut commented Sep 16, 2016

Now source code is more readable and easier to understand.

You think ? No, I'm joking. Huge work here. Files path looks good, client and on the server.

I've tested it and :

Sadness, I don't see any clue to explain that behavior

@cgalvarez
Copy link
Author

Ok, I will take a look at those issues of yours, and a couple of mine what appeared when tested my package :trollface: (I didn't see them because only tested with your package).

I'm going to improve this package coverage if I can too.

@serut serut merged commit 7c040ad into serut:dev-1.0.0 Sep 16, 2016
@cgalvarez cgalvarez mentioned this pull request Sep 17, 2016
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.

None yet

4 participants