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

Integrate test documentation from Homestead docs #284

Merged
merged 6 commits into from
Aug 29, 2017

Conversation

holgerd77
Copy link
Contributor

Implementation of #272

I've now integrated Homestead tests doc files in a docs folder in the repository and also added some doc contribution notes.

I've already deployed this on ReadTheDocs, this is still pointing to my fork but can be easily switched over.

@holgerd77
Copy link
Contributor Author

@pirapira @winsvega I could easily add you as administrators on ReadTheDocs.org (and also remove myself), you need an account there though.

@pirapira
Copy link
Member

@holgerd77 I created an account under "pirapira" and connected it with this GitHub @pirapira.

@holgerd77
Copy link
Contributor Author

@pirapira Ok, added you.

@winsvega
Copy link
Collaborator

what about the docs in cpp-ethereum repo?

@pirapira
Copy link
Member

@winsvega the doc in cpp-ethereum describes testeth. The document here describes the test formats.

@pirapira
Copy link
Member

@holgerd77 ready for review?

@holgerd77
Copy link
Contributor Author

@pirapira Yes, I was thinking about directly improving the docs a bit as well, but I'll probably do this in a separate PR. Think its good to have this integrated with one plain PR just taking over the docs as is.

Found in ``/BlockTests``, the blockchain tests aim is to test the basic verification of a blockchain.

``/BlockTests`` - general blockchain tests. All blocks are built on network: **Frontier**
``/BlockTests/Homestead`` - homestead blockchain tests. All blocks are built on network: **Homestead**
Copy link
Collaborator

Choose a reason for hiding this comment

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

this info is outdated. blockchain folder structure is different now. network rules are read from networ field from within the test

Copy link
Member

@pirapira pirapira left a comment

Choose a reason for hiding this comment

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

Looks good as the first documentation commit. Details can be improved in parallel.

"pre": { ... },
"blocks" : [
{
"chainname" : "A",
Copy link
Collaborator

Choose a reason for hiding this comment

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

optional field. we should leave it only in the test fillers.
additional or optional fields in the final tests are confusing other teams

"blocks" : [
{
"chainname" : "A",
"blocknumber" : "1",
Copy link
Collaborator

Choose a reason for hiding this comment

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

filleronly field

``uncleHash``:
The Keccak 256-bit hash of the uncles list portion of this block

* ``pre`` section: as described in State Tests.
Copy link
Collaborator

Choose a reason for hiding this comment

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

describe network field somewhere here


* ``uncleHeaders`` section is a list of block headers which have the same format as descibed in `genesisBlockHeader`.

Optional BlockHeader Sections (Information fields)
Copy link
Collaborator

Choose a reason for hiding this comment

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

should be only in fillers

.. _contribute:

==================
Contribute to Docs
Copy link
Collaborator

Choose a reason for hiding this comment

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

sphinx has a bug where it might compile docs and not create some code blocks, quotes or tables. better view this scripts in github

.. _difficulty_tests:

################################################################################
Difficulty Tests
Copy link
Collaborator

Choose a reason for hiding this comment

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

this tests are valid. but we have to add that it is not maintained and not being executed on hive

.. _state_tests:

################################################################################
State Tests
Copy link
Collaborator

Choose a reason for hiding this comment

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

this tests has been changed drastically new schema is here
ethereum/EIPs#176 (might be slightly outdated. new field is logs hash)

@@ -0,0 +1,68 @@

*****************************************************
Using Testeth
Copy link
Collaborator

Choose a reason for hiding this comment

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

this file is updated at cpp-ethereum docs

.. _vm_tests:

################################################################################
VM Tests
Copy link
Collaborator

Choose a reason for hiding this comment

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

@pirapira maintain those tests

@winsvega
Copy link
Collaborator

speaking of details. added some comments

@pirapira
Copy link
Member

pirapira commented Aug 29, 2017

@holgerd77 now that @winsvega has reviewed it, do you want to apply his comments in this PR?

@holgerd77
Copy link
Contributor Author

@pirapira @winsvega Ok, will do, maybe it makes sense to have a more solid foundation to start from with the moved docs. Don't expect half of a rewrite though, can spend limited time on it.

@pirapira
Copy link
Member

@winsvega let's delay the second-round fixes after merging this PR.

@winsvega
Copy link
Collaborator

ok

@holgerd77
Copy link
Contributor Author

@pirapira Ok, this feels also more relaxed for me, then I'll help out on improving things separately when I find some time in new PR(s) and also look into some of the issues from @winsvega.

@pirapira
Copy link
Member

@holgerd77 I just protected your PR from further review comments. I heard you would address the existing comments, so tell me if you've changed mind about this.

@holgerd77
Copy link
Contributor Author

@pirapira Ah, sorry, misunderstanding, thought you meant the existing comments with "second-round" ("first-round" being the doc extraction :-)). Ok, no, then I'll do additional commits for the outstanding comments.

@holgerd77
Copy link
Contributor Author

@pirapira @winsvega I've made some updates to the blockchain tests and the state tests section, but it feels a bit "wrong" to do this I have to admit, since by starting to update these docs so broadly one is automatically drawn into a half-rewrite without knowing where to start and where to stop, since there is so much outdated information.

Please let's just merge this "as is", then the changes itself can be adressed by various people on separate PRs.

@pirapira pirapira merged commit d08cb18 into ethereum:develop Aug 29, 2017
@pirapira
Copy link
Member

@winsvega will you file a follow-up PR containing the edits you suggested?

@holgerd77 holgerd77 deleted the add-docs branch August 30, 2017 16:00
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.

3 participants