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

docs: Clarifying testing instructions #15127

Merged
merged 1 commit into from Jan 9, 2019

Conversation

Projects
None yet
8 participants
@benthecarman
Copy link
Contributor

commented Jan 8, 2019

This statement confused me on my first time reading through. Hopefully, this addition will help someone else on their first time.

@fanquake fanquake added the Docs label Jan 8, 2019

@sipa

This comment has been minimized.

Copy link
Member

commented Jan 8, 2019

Is this needed? I never use those options

@benthecarman

This comment has been minimized.

Copy link
Contributor Author

commented Jan 8, 2019

Is this needed? I never use those options

If that's true it might be better to change it to say something like "Be sure not to disable wallet, utils, and daemon, the default is recommended."

@fanquake

This comment has been minimized.

Copy link
Member

commented Jan 8, 2019

If the sentence is confusing I'd rather we just remove it. I don't think we need to add explicit documentation for options that are enabled by default.

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 8, 2019

Also tend to NACK. The tests are built by default, on purpose. It is a matter of following the build steps for whatever platform, no need to duplicate the configure command-line here.

@benthecarman benthecarman force-pushed the benthecarman:docs-clarifying-test-notes branch Jan 8, 2019

@benthecarman

This comment has been minimized.

Copy link
Contributor Author

commented Jan 8, 2019

Changed it so it now gives a link to build instructions instead

@jnewbery

This comment has been minimized.

Copy link
Member

commented Jan 8, 2019

"Before tests can be run locally, your system must be built." isn't correct. You need to build bitcoind for your system (eg linux, macos, etc), not build your system.

ACK adding a reference to the build instructions.

@benthecarman benthecarman force-pushed the benthecarman:docs-clarifying-test-notes branch 2 times, most recently Jan 8, 2019

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Jan 8, 2019

ACK 127e37e7fcd426c990a6e4c49af9d1c35e76d384

@fanquake

This comment has been minimized.

Copy link
Member

commented Jan 8, 2019

utACK 127e37e

Show resolved Hide resolved test/README.md Outdated

@benthecarman benthecarman force-pushed the benthecarman:docs-clarifying-test-notes branch to ef5ebc6 Jan 9, 2019

@jnewbery

This comment has been minimized.

Copy link
Member

commented Jan 9, 2019

ACK ef5ebc6

@hebasto

This comment has been minimized.

Copy link
Member

commented Jan 9, 2019

utACK ef5ebc6

@MarcoFalke MarcoFalke merged commit ef5ebc6 into bitcoin:master Jan 9, 2019

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

MarcoFalke added a commit that referenced this pull request Jan 9, 2019

Merge #15127: docs: Clarifying testing instructions
ef5ebc6 docs: Clarifying testing instructions (benthecarman)

Pull request description:

  This statement confused me on my first time reading through.  Hopefully, this addition will help someone else on their first time.

Tree-SHA512: 17f421275adb7586eca954910269d29fcd3bacc42fab4bc2e01110f9e13ca6f8c1ca178246f7192e1131f14ced7f7dc0b57e7aec324898807c1813a2ebc513de

@benthecarman benthecarman deleted the benthecarman:docs-clarifying-test-notes branch Feb 8, 2019

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.