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(docs): include all parameters in test parameter datatables #842

Merged

Conversation

danceratopz
Copy link
Member

@danceratopz danceratopz commented Sep 26, 2024

🗒️ Description

The test function parameter tables introduced in #801 try to only show "relevant" test function parameters. It's a nice idea, but I think this obfuscates the fact that we always parametrize by fork and potentially fixture type, so I think it's better to be transparent about that but limit the number of entries displayed by using additional select elements with reasonable defaults.

Preview available at pr-842-preview.

Example:
image

🔗 Related Issues

Done:

  • fix(docs): move initialization js from jinja2 template to site.js #813
  • Generate test function parameters for all forks by running fill with --until={GENERATE_UNTIL_FORK} instead of --fork=....
  • Specify a target fork that can be used to display a default set of parameters (--gen-docs-target-fork={TARGET_FORK}.
  • Tooltip upon hover on Test Case ID entry.
  • Copy-paste the full test id from the table via left-click on Test Case ID entry.
  • feat(docs): make test functions discoverable by full test node id and test function parameter ids #814 (not yet, we could add a hidden table column to display make entries searchable by full nodide).
  • Fix following bug (by using datables API in select elements in order to correctly update in the table entries?):
    • The selectors work and update the rows in the table, but:
      • If I search in the "Search" box, I get output like "
        Showing 1 to 20 of 20 entries (filtered from 48 total entries)", and if there's no need to paginate, the values are shown on the first page.
      • If I filter using fork or fixture type, the rows are filtered, but the output doesn't change (it stays at "Showing 1 to 20 of 20 entries" and the pagination doesn't update; in the worst case the table is empty and you have to click through the pages until you find where the values are in the table (two example screenshots attached for the same selector values).
  • Arrange select elements more nicely, apply styling.

✅ Checklist

  • All: Set appropriate labels for the changes.
  • All: Considered squashing commits to improve commit history.
  • All: Added an entry to CHANGELOG.md.
  • All: Considered updating the online docs in the ./docs/ directory.
  • Tests: All converted JSON/YML tests from ethereum/tests have been added to converted-ethereum-tests.txt.
  • Tests: A PR with removal of converted JSON/YML tests from ethereum/tests have been opened.
  • Tests: Included the type and version of evm t8n tool used to locally execute test cases: e.g., ref with commit hash or geth 1.13.1-stable-3f40e65.
  • Tests: Ran mkdocs serve locally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.

@danceratopz danceratopz added scope:docs Scope: Documentation type:feat type: Feature labels Sep 26, 2024
@danceratopz danceratopz force-pushed the docs/include-all-parameters-in-test-parameter-datatables branch from 563d6d3 to a8a8aa2 Compare September 27, 2024 09:30
@danceratopz danceratopz requested a review from marioevz October 3, 2024 12:30
@danceratopz danceratopz force-pushed the docs/include-all-parameters-in-test-parameter-datatables branch 2 times, most recently from 81dbe64 to 3e96532 Compare October 3, 2024 12:53
@danceratopz danceratopz marked this pull request as ready for review October 3, 2024 13:01
@danceratopz danceratopz requested review from spencer-tb and removed request for marioevz October 3, 2024 14:50
danceratopz and others added 20 commits October 8, 2024 15:21
Also add a tooltip to display the full test id in parameter tables using abbrev.
Previously, the test function parameter tables tried to show parameter names without any fork or fixture type info, i.e., just the parameters relevant to the test function. The test case ID was abbreviated and the `fork_*` and fixture type (e.g.,`state_test`) was removed from the ID shown in the table. This kinda made sense, but obfusacates the actual test case ID and how the function is actually parametrized, which could lead to confusion.

We now collect tests over full range of forks and the rows of test cases for a test function now in parameter tables now reflect the actual list of cases as collected by pytest. In order to keep the tables useful, selectors have been added for fork and fixture type. By default, only tests for the new command-line option --gen-docs-target-fork value are shown in the table.
Ensure that GEN_TEST_DOC_VERSION is set in tox_verify; needs test for doc_*.yaml workflows.
danceratopz and others added 11 commits October 8, 2024 15:23
- Inlude the table, even if the test function is only parametrized by fork (and output fixture format).
- Simplify fill command admonitions: Only show the command to generate the fixture for the target fork if `valid_from` is deployed, otherwise, use the `valid_from` val.
- Test module pages: Test function case count is taken from the target fork if `valid_from` is deployed, otherwise, use the `valid_from` val.
@marioevz marioevz force-pushed the docs/include-all-parameters-in-test-parameter-datatables branch from dd1cc34 to 816f0df Compare October 8, 2024 15:33
@marioevz
Copy link
Member

marioevz commented Oct 8, 2024

Rebased on top of Osaka, waiting on passing CI before merging.

Copy link
Member

@marioevz marioevz left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this @raxhvl and @danceratopz !
I verified this after the rebase and worked nicely out of the box 👍

@marioevz marioevz merged commit e508623 into main Oct 8, 2024
5 checks passed
@marioevz marioevz deleted the docs/include-all-parameters-in-test-parameter-datatables branch October 8, 2024 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope:docs Scope: Documentation type:feat type: Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants