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

Remove npm #1943

Closed
wants to merge 6 commits into from

Conversation

Projects
None yet
6 participants
@mellery451
Copy link
Contributor

commented Dec 22, 2016

Eliminate npm tests and remove test directory. The second commit is just a delete, so only 7dd5eda needs to be reviewed.

"scripts": {
"pretest": "node test/pretest.js",
"test": "mocha test/websocket-test.js test/server-test.js test/*-test.{js,coffee}"
},

This comment has been minimized.

Copy link
@ximinez

ximinez Dec 22, 2016

Contributor

I think this file is only used in the context of npm, so it can be deleted, too.

This comment has been minimized.

Copy link
@mellery451

mellery451 Dec 22, 2016

Author Contributor

will remove

@mellery451 mellery451 force-pushed the mellery451:remove_npm branch from 6c08ae8 to c0904e1 Dec 22, 2016

@ximinez

This comment has been minimized.

Copy link
Contributor

commented Dec 22, 2016

@mellery451 Here is the fix for the Appveyor build fcd38db

@codecov-io

This comment has been minimized.

Copy link

commented Dec 22, 2016

Current coverage is 66.50% (diff: 100%)

Merging #1943 into develop will decrease coverage by <.01%

@@            develop      #1943   diff @@
==========================================
  Files           687        687          
  Lines         49314      49314          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits          32801      32797     -4   
- Misses        16513      16517     +4   
  Partials          0          0          

Powered by Codecov. Last update 87273e2...741149f

@vinniefalco vinniefalco self-assigned this Dec 22, 2016

@@ -194,7 +194,7 @@ class sync_test : public beast::unit_test::suite

BEAST_EXPECT(source.deepCompare (destination));

std::cerr << "Checking destination invariants" << std::endl;

This comment has been minimized.

Copy link
@vinniefalco

vinniefalco Dec 22, 2016

Contributor

Are there any other uses of std::cerr in the code base?

This comment has been minimized.

Copy link
@mellery451

mellery451 Dec 22, 2016

Author Contributor

yeah - I see about 30 hits for ::cerr in src/ripple

@vinniefalco

This comment has been minimized.

Copy link
Contributor

commented Dec 22, 2016

Spelling: "Apveyor" in commit message

@ximinez

This comment has been minimized.

Copy link
Contributor

commented Dec 22, 2016

Ugh. I'm all in favor of squashing that commit down, though, so it doesn't need to be fixed.

@mellery451

This comment has been minimized.

Copy link
Contributor Author

commented Dec 22, 2016

@ximinez -- will do

@vinniefalco

This comment has been minimized.

Copy link
Contributor

commented Dec 22, 2016

@ximinez Oh... true.

@mellery451 mellery451 force-pushed the mellery451:remove_npm branch from 876dcdb to f23db26 Dec 22, 2016

@ximinez
Copy link
Contributor

left a comment

Conditional approval depending on the answer to the PKGBUILD question, which I am not set up to test myself.

@@ -25,9 +25,6 @@ build() {
}

check() {
cd "$srcdir/$pkgname"

This comment has been minimized.

Copy link
@ximinez

ximinez Dec 22, 2016

Contributor

Will the unittests still work without changing dir?

This comment has been minimized.

Copy link
@mellery451

mellery451 Dec 22, 2016

Author Contributor

good point...@wilsonianb, how can I vet changes to PKGBUILD ?

This comment has been minimized.

Copy link
@wilsonianb

wilsonianb Dec 23, 2016

I'm not familiar with Arch Linux / PKGBUILD, but I think you are right to keep the cd

@ximinez
Copy link
Contributor

left a comment

Something I missed - In Builds/VisualStudio2015/README.md, the "Prerequisites" section has a link to "Node.js" that no longer goes anywhere.

...And the "Unit Tests (Recommended)" section no longer really needs the "Internal" subhead, since there's only one now.

@ximinez
Copy link
Contributor

left a comment

Looks good. I'm so excited to finally be done with these tests. 👍

@mellery451 mellery451 force-pushed the mellery451:remove_npm branch from 381aa5f to aa4bdf0 Dec 27, 2016

@mellery451 mellery451 added the Passed label Jan 3, 2017

@mellery451 mellery451 force-pushed the mellery451:remove_npm branch from aa4bdf0 to a3fa012 Jan 3, 2017

mellery451 added some commits Dec 21, 2016

Eliminate npm tests (RIPD-1369)
Remove mention of npm tests in developer docs. Eliminate `npm test` from
automation and ci scripts.
[FOLD] Improve AppVeyor scripts (CI):
* Abort PS script if command fails
* Do not fail build if unit tests output to stderr
* Change Config loading message to stdout

@mellery451 mellery451 force-pushed the mellery451:remove_npm branch from a3fa012 to 2eaf501 Jan 10, 2017

@wilsonianb

This comment has been minimized.

Copy link

commented Jan 13, 2017

@mellery451

This comment has been minimized.

Copy link
Contributor Author

commented Jan 13, 2017

@wilsonianb - removed those nodejs mentions...thanks.

@wilsonianb

This comment has been minimized.

Copy link

commented Jan 13, 2017

👍

@nbougalis

This comment has been minimized.

Copy link
Member

commented Jan 14, 2017

Merged as e3ff306. And there was much rejoicing.

@nbougalis nbougalis closed this Jan 14, 2017

@mellery451 mellery451 deleted the mellery451:remove_npm branch Jan 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.