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

feat: add scripts option to lookup table #696

Merged
merged 2 commits into from
Apr 13, 2019

Conversation

richardlau
Copy link
Member

If set, "scripts" specifies the scripts from the module's package.json
to run instead of the default "test".

Allows nan to be added (second commit).

Checklist
  • npm test passes
  • tests are included
  • documentation is changed or added
  • contribution guidelines followed
    here

@richardlau

This comment has been minimized.

@codecov-io
Copy link

codecov-io commented Apr 5, 2019

Codecov Report

Merging #696 into master will increase coverage by 0.11%.
The diff coverage is 97.22%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #696      +/-   ##
==========================================
+ Coverage   94.64%   94.76%   +0.11%     
==========================================
  Files          27       27              
  Lines         859      878      +19     
==========================================
+ Hits          813      832      +19     
  Misses         46       46
Impacted Files Coverage Δ
lib/lookup.js 97.46% <100%> (+0.09%) ⬆️
lib/package-manager/test.js 98.52% <96.96%> (+0.45%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 36d8dd2...287a221. Read the comment docs.

@richardlau
Copy link
Member Author

richardlau commented Apr 5, 2019

CI for this PR running citgm nan: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker-pipeline/68/

Rerun with fixes for Node.js 6 and 8 (avoid use of array.values()):
https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker-pipeline/69/console

Almost clean on 6 and 8 (linux-s390x ran on a rebuilt host which appears to be missing curl used to download Node.js).

Building the nan test bindings on Windows is failing for 10 and 11 -- I'm not entirely sure why but they do run on different workers (with VS2017) than the Windows builds for 6 and 8 (VS2015) which passed.

 > node-gyp rebuild --msvs_version=2015 --directory test
 C:\workspace\citgm-smoker-nobuild\workspacecitgm-smoker-nobuild\citgm_tmp\54a49ab2-3776-45c0-a8d3-9b5cbc5e27b4\nan>if not defined npm_config_node_gyp (node "C:\workspace\citgm-smoker-nobuild\node\node_modules\npm\node_modules\npm-lifecycle\node-gyp-bin\\..\..\node_modules\node-gyp\bin\node-gyp.js" rebuild --msvs_version=2015 --directory test )  else (node "C:\workspace\citgm-smoker-nobuild\node\node_modules\npm\node_modules\node-gyp\bin\node-gyp.js" rebuild --msvs_version=2015 --directory test ) 
 Building the projects in this solution one at a time. To enable parallel build, please add the "/m" switch.
 C:\workspace\citgm-smoker-nobuild\workspacecitgm-smoker-nobuild\citgm_tmp\54a49ab2-3776-45c0-a8d3-9b5cbc5e27b4\nan\test\build\accessors.vcxproj(20,3): error MSB4019: The imported project "C:\Microsoft.Cpp.Default.props" was not found. Confirm that the path in the <Import> declaration is correct, and that the file exists on disk.
...

@targos
Copy link
Member

targos commented Apr 5, 2019

@SimenB Is that also what you need for Jest?

@SimenB
Copy link
Member

SimenB commented Apr 5, 2019

Yes, perfect!

@SimenB SimenB mentioned this pull request Apr 5, 2019
lib/package-manager/test.js Outdated Show resolved Hide resolved
If set, "scripts" specifies the scripts from the module's package.json
to run instead of the default "test".
@targos
Copy link
Member

targos commented Apr 13, 2019

@richardlau can we go ahead with this?

@richardlau richardlau merged commit 75068e9 into nodejs:master Apr 13, 2019
@richardlau
Copy link
Member Author

richardlau commented Apr 13, 2019

I released a patch release just before landing this. Currently waiting for Travis CI to complete on the rebased #706 and then I'll land that too and release a new minor that includes this PR.

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

Successfully merging this pull request may close these issues.

None yet

5 participants