Move test creation docs to testing documentation #539
Conversation
|
Will do two additional PRs on Aleth and testeth to deprecate the currently doubled documentation on both libraries. On testeth we should on top add a prominent additional note that the tool currently is not usable until eventually further developed. |
docs/generating-tests.rst
Outdated
|
|
||
| .. note:: | ||
| If ``testeth`` is looking for tests in the ``../../test/jsontests`` directory, | ||
| you have probably not specified the ``--testpath`` option. |
There was a problem hiding this comment.
I don't see any --testpath option in the instructions above.
There was a problem hiding this comment.
by default test path is read from environment variable
ETHEREUM_TEST_PATH="/home/wins/Ethereum/tests"
if its not set the default path is relative to aleth build directory.
--testpath option override the above.
There was a problem hiding this comment.
Have added a note.
docs/generating-tests.rst
Outdated
|
|
||
| .. code:: bash | ||
|
|
||
| ETHEREUM_TEST_PATH="<LOCAL_PATH_TO_ETH_TESTS>" test/testeth -t GeneralStateTests/stExample2 -- --filltests --checkstate |
There was a problem hiding this comment.
A build of testeth from the current Aleth master doesn't appear to support --checkstate.
There was a problem hiding this comment.
@winsvega What is the current state of this? Has this been replaced by something else? Or should I just remove this from the documentation?
There was a problem hiding this comment.
--checkstate is always enabled by default. the option removed.
There was a problem hiding this comment.
stExample2 actually might be too complicated. currently it requires you to build aleth and modify testeth to understand the stExample2. I suggest to limit this doc to use only existing test suites.
There was a problem hiding this comment.
Already stating my changes here, PR update will follow once I am through (this also goes for the other comments within the next hour):
Have removed --checkstate and modified the text a bit so that this more applies for both creating a new filler as well as modifying an existing one.
docs/generating-tests.rst
Outdated
| ``testeth`` with ``--filltests`` fills every test filler it finds. The command might | ||
| modify existing test cases. After running ``testeth`` with ``--filltests``\ , try running | ||
| ``git status`` in the ``tests`` directory. If ``git status`` indicates changes in | ||
| unexpected files, that is an indication that the behavior of ``cpp-ethereum`` changed unexpectedly. |
There was a problem hiding this comment.
Since the filled tests include the version of testeth and lllc used to fill them, if you built them from source --filltests will likely change every file from that directory of tests. You can sort through them fairly easily looking at the diff but it may be better to explain the --singletest option here so only the newly added test is filled.
There was a problem hiding this comment.
Travis CI seems to be doing a check of the sourceHash in each filled file - it may be useful to explain what that command is as a way to identify which tests need to be refilled.
There was a problem hiding this comment.
I am not having a local up-to-date build of testeth up an running and so can't try it out myself, could you make a concrete suggestion for a singletest option addition text? Think that would be easiest.
Travis is using the test.py Python script to check, this is not part of the Aleth client (@winsvega: correct me if I am wrong). One should probably add to the docs that this Python tool exists in a addition also here and point to the test.py docs (currently in README, should be moved to the docs as well though).
There was a problem hiding this comment.
test.py double check the test to be filled from latest fillers. when you run the --filltests command it will be checked for errors (expect section) automatically.
you only use --filltests command when developing new test cases. if you just running the tests you must not use this command.
--singletest <TESTNAME> --singlenet <FORKNAME> allow to select a specific test at specific fork. -d <DATAINDEX> -g <GASINDEX> -v <VALUEINDEX> allow to select specific transaction from general state test.
There was a problem hiding this comment.
Have added these extra explanations for --singletest.
There was a problem hiding this comment.
Maybe:
Alternatively, individual tests can be refilled using:
ETHEREUM_TEST_PATH="<LOCAL_PATH_TO_ETH_TESTS>" test/testeth -t GeneralStateTests/stExample2 -- --filltests --singletest <test-name>
docs/generating-tests.rst
Outdated
| ``git add test/jsontests`` followed by ``git commit``. | ||
|
|
||
| When you file this commit as a pull-request to ``ethereum/aleth``, Travis CI | ||
| should try the newly filled tests. |
There was a problem hiding this comment.
If I understand this correctly, the aim is the get the Aleth Travis CI build to run the new tests with Aleth to check they pass. To do that a PR is submitted to Aleth that updates the git submodule for ethereum/tests to point to a branch with the new tests. It may be worth giving an overview like that to explain what we're trying to do before launching into how. I had to read it a few times to understand it without the context.
It should also say how you get in touch with someone who can create the branch in ethereum/tests for you.
Even better would be to setup a CI job so that this test is just automatically run for each PR submitted to ethereum/tests. Having to get someone to create a branch in ethereum/tests for you is a pretty big barrier.
There was a problem hiding this comment.
Will try to do a summary of this in the documentation. Will think about the implications and how the process is currently handled and could be eventually improved when I work on the doc section update.
There was a problem hiding this comment.
aleth is a legacy actually. the logic you described is aleth specific. the actuall test repo should not really care about aleth. I've been working on migrating the test logic from aleth into a separate test tool. one we release the hardfork the priority would be to make a production version of that tool that would be capable of generating/running the tests from test repo using any compatible client.
There was a problem hiding this comment.
Clarified the whole context by doing additions from both of your comments in the section.
| After these two commands, | ||
|
|
||
|
|
||
| * ``git status`` to check if any GeneralStateTest has changed. If yes, revert the changes, and follow section _\ ``Trying the Filled Test Locally``. That will probably reveail an error that you need to debug. |
There was a problem hiding this comment.
This will also be affected by the way the testeth and lllc version is included in the generated files. It works out ok for new tests though since they turn up as new files and the other changes are all to existing files.
Given the move to specific releases for the reference tests, I wonder if it makes sense to leave generating the blockchain versions of tests to a one-off run before taking the release.
There was a problem hiding this comment.
Will leave this as a suggestion here for now, maybe @winsvega wants to add on this as well.
There was a problem hiding this comment.
I suggest to remove stExapmle2 from this doc. it is too complicated and requires you to change the aleth and rebuild it.
This lines also does not assume the test hash and versions to be updated on the test files. so the logic of this part is no longer valid.
There was a problem hiding this comment.
Will leave this for now. I think this is due to a larger rewrite which can be done in follow-up PRs.
docs/generating-tests.rst
Outdated
| where the environmental variable ``ETHEREUM_TEST_PATH`` should point to the directory | ||
| where ``tests`` repository is checked out. ``stExample2`` should be replaced with | ||
| the name of the subdirectory you are working on. ``--filltests`` option tells ``testeth`` | ||
| to fill tests. ``--checkstate`` tells ``testeth`` to check the final states against |
There was a problem hiding this comment.
Note to self: this part has to be updated as well depending on the answer from @winsvega above.
There was a problem hiding this comment.
--checkstate is depricated. it is checking final state every time you fill the test now.
There was a problem hiding this comment.
Ok, have removed --checkstate here as well.
|
Hi @winsvega, can you give short answers on the comments above? I would then update this PR and would like to have this merged quickly, then we can do subsequent doc updates on a smaller scale. |
|
Note to self:
|
|
there is also Yoichi's guide on how to create a new test case. that should be checked and added to the docs also. |
|
@winsvega Thanks, will submit an updated PR within the next hour and include your answers in the docs. What/where is this guide from Yoichi? Do you have got a URL for that? |
docs/generating-tests.rst
Outdated
| are test cases for all Ethereum implementations. The test cases are distributed | ||
| in the "filled" form, which contains, for example, the expected state root hash after transactions. | ||
| The filled test cases are usually not written by hand, but generated from "test filler" files. | ||
| ``testeth`` executable in cpp-ethereum can convert test fillers into test cases. |
There was a problem hiding this comment.
cpp-ethereum has been renamed to aleth there are some places in this document which still mentions cpp-ethereum.
There was a problem hiding this comment.
Thanks Hugo @hugo-dc, have updated all remaining references.
de8b920 to
cdf5a2d
Compare
|
@winsvega Ok, did the changes, this should be ready for merge now. |
|
Hi @ajsutton, since we are low on people writing tests atm and client devs are waiting for Constantinople tests to be finished, would you be open to write 2-3 of the That would be really appreciated if somehow possible 😄! |
|
@hugo What's with you, are you up to start working on one or two tests for this? |
|
Well this manual in the PR is basically what was initially Yoichi's guide Uncovered test cases list for EXTCODEHASH: |
docs/generating-tests.rst
Outdated
| .. [#] The file name and the name written in JSON should match because ``testeth`` prints the name written in JSON, but the user needs to find a file. | ||
|
|
||
|
|
||
| ``env`` field contains some parameters in a straightforward way. |
There was a problem hiding this comment.
its been described in readthedocs somewhere. what that section means.
There was a problem hiding this comment.
Yes, that's in the state test section, I've added a cross-reference, thanks (again, like with the other updates, will publish the updated version within the next 1-2 hours after these comments).
docs/generating-tests.rst
Outdated
| "0x0f572e5295c57f15886f9b263e2f6d2d6c7b5ec6" : { | ||
| "balance" : "0x0de0b6b3a7640000", | ||
| "code" : "{ (MSTORE 0 0x112233445566778899aabbccddeeff) (RETURNDATACOPY 0 0 32) (SSTORE 0 (MLOAD 0)) }", | ||
| "code" : "0x306000526020600060003e600051600055", |
There was a problem hiding this comment.
this line should be removed
docs/generating-tests.rst
Outdated
|
|
||
| As specified in the Yellow Paper, an account contains a balance, a code, a nonce and a storage. | ||
|
|
||
| Notice the ``code`` field is duplicated. If many fields exist under the same name, the last one is used. In this particular case, the LLL compiler was not ready to parse the new instruction ``RETURNDATACOPY`` so a compiled runtime bytecode is added as the second ``code`` field [#]_. |
There was a problem hiding this comment.
duplicates were used as a comments. now to make a comment in json any field starting with // is threated as comment.
There was a problem hiding this comment.
Ok, I've added an example comment to the source extract and adopted the text accordingly.
| "gas" : -1, | ||
| "value" : -1 | ||
| }, | ||
| "network" : ["Frontier", "Homestead"], |
There was a problem hiding this comment.
network names / forks been extended to format
>=Homestead
the order of forks: Frontier < Homestead < EIP150 < EIP158 < Byzantium < Constantinople
There was a problem hiding this comment.
Ok, have done additions to network format explanation and an extra note on fork order.
Yes, I'm looking at the current test cases for EXTCODEHASH right now. |
docs/generating-tests.rst
Outdated
| ``indexes`` field specifies a subset of the transactions. ``-1`` means "whichever". | ||
| ``"data" : 0`` points to the first element in the ``data`` field in ``transaction``. | ||
|
|
||
| ``network`` field is somehow similar. It specifies the versions of the protocol for which the expectation applies. For expectations common to all versions, say ``"network" : ALL``. |
There was a problem hiding this comment.
network ALL not supported. ALL equal to >=Frontier now
docs/generating-tests.rst
Outdated
|
|
||
| .. code:: | ||
|
|
||
| ETHEREUM_TEST_PATH="../../tests" test/testeth -t GeneralStateTests/stExample2 -- --filltests --fillchain |
There was a problem hiding this comment.
I remember reports that relative path does not work like this.
ETHEREUM_TEST_PATH should be set absolute and inside environment once.
or you could set it / override using --testpath "" argument.
There was a problem hiding this comment.
Whew, that's a biggie! Have replaced all the inline pre-pended paths commands and added a note on all occurences to have a look at the unified section explaining how to set ETHEREUM_TEST_PATH.
| "currentCoinbase" : <an address>, | ||
| "currentDifficulty" : "0x020000", | ||
| "currentGasLimit" : <anything < 2**63-1 but make sure the transaction does not hit>, | ||
| "currentNumber" : "1", |
There was a problem hiding this comment.
for generating blockchain tests version this field must be equal to 1 and timestamp to 1000
actually env might become depricated in state test fillers.
There was a problem hiding this comment.
Ok, have added both notes in appropriate places in the docs.
|
@winsvega Thanks for all the comments, will do an update including all your changes tomorrow morning! |
|
Btw, @hugo-dc also created some nice documentation, probably worth reading and copying over some parts: https://github.com/ewasm/tests/blob/wasm-tests/src/GeneralStateTestsFiller/stEWASMTests/README.md |
|
Ah, that's something for @hugo-dc in a subsequent PR! 😄 |
I'm travelling from Prague back to Brisbane today but could pick some up early next week. |
cdf5a2d to
227bbfa
Compare
|
Ok, have updated. I think will do further iteration on the docs, but will do this in subsequent PRs, so this should be ready for merge now. |
|
@pipermerriam Automatic doc generation triggering works like a charm, as a side node! 😄 |
|
related: #513 |
|
There appears to be an issue. People are confused with state tests now requiring coinbase touch simulation (act as a blockchain tests) Could this be added to docs also? |
|
Will do another documentation round in the upcoming days. This week is busy with VM release, not sure if I'll make it, but latest early next week. |
This PR moves the documentation on test generation to the generated testing docs. This should allow us to iterate faster with documentation updates and also makes this more independent from the current test tool in use, being it Aleth itself, eventually an updated testeth version or something else in the future.
I've also done some documentation updates, this can nevetheless just be a starter.