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

testing: extend regression tests to item analyzers #519

Merged
merged 1 commit into from
Sep 23, 2023

Conversation

Andrew7234
Copy link
Collaborator

@Andrew7234 Andrew7234 commented Sep 14, 2023

Task

From #410 (comment):

Our non-block-based analyzers also have a fair bit of complexity in them, and it would be good to test them for e2e regressions also; currently, we only test the block-based analyzers.

We can probably fit all analyzers except the git-based metadata_registry into our existing regression framework. I think what we want to do is have a way to run all the block analyzers to the end, then run all the evm_tokens_* and evm_token_balances_* analyzers and the aggregate_stats analyzer. (That way, they'll query the node at a predictable height, so they can always hit the cache.) The first part is easy; for the second part, we don't have a way yet of telling the analyzers "exit as soon as there is no more work for you"; add it in the scope of this work.

The second part would ideally happen _after_ because it will be cleaner to make this change once in a framework, rather than copy-pasted across 5 analyzers.

This PR

Tests functionality of the evm_tokens, evm_token_balances, and evm_contracts analyzers

@Andrew7234 Andrew7234 force-pushed the andrew7234/cache-node-errors branch from 90974ee to bdb6e47 Compare September 14, 2023 18:30
@Andrew7234 Andrew7234 force-pushed the andrew7234/item-analyzer-regression-tests branch from 5b0c749 to fc36464 Compare September 14, 2023 18:31
Copy link
Contributor

@mitjat mitjat left a comment

Choose a reason for hiding this comment

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

Ooh I'm really looking forward to this :)

tests/e2e_regression/run.sh Outdated Show resolved Hide resolved
tests/e2e_regression/e2e_config_2.yml Outdated Show resolved Hide resolved
@@ -0,0 +1,42 @@
analysis:
Copy link
Contributor

@mitjat mitjat Sep 14, 2023

Choose a reason for hiding this comment

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

I'm worried about the _1 and _2 config files drifting apart, leading to hard-to-understand failures in e2e tests.

One way to avoid it would be to derive them programmatically from some common template. But I think it would be relatively verbose to do it, and potentially awkward to maintain.

A more light-weight option is to add a step to the test itself that makes sure the two configs are both talking to the same cache, both expecting mainnet (vs testnet), etc. Something like this:

#!/bin/bash

set -euo pipefail

# Our e2e regression test runs the analyzer twice in close succession. This is a slightly hacky but
# simple way to ensure block analyzers run first, and non-block analyzers perform EVM queries always
# at the same height, thereby hitting the offline response cache.
#
# This script compares the key parameters of the two config files used in the two runs. If any of
# those parameters differ, it shows the diff and exits with an error.

# Elements of the config files that we'll compare
important_attrs='{"cache": .analysis.source.cache.cache_dir, "chain_name": .analysis.source.chain_name, "db": .analysis.storage.endpoint}'

# A converter whose only dependency is stock python, which is likely to be preinstalled
alias yaml2json="python -c 'import sys,yaml,json; print(json.dumps(yaml.safe_load(str(sys.stdin.read()))))'" 

# Compare
cat tests/e2e_regression/e2e_config_1.yml | yaml2json | jq "$important_attrs" > /tmp/e2e_config_1.summary
cat tests/e2e_regression/e2e_config_2.yml | yaml2json | jq "$important_attrs" > /tmp/e2e_config_2.summary
diff /tmp/e2e_config_1.summary /tmp/e2e_config_2.summary || { echo "The two config files for e2e tests differ in key parameters! See diff above."; exit 1; }

Then in the Makefile, at the beginning of the regression test target, we just need to call it:

@ensure_consistent_config.sh

The script is untested apart from the jq part. Please rethink important_config with a critical eye.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

got it, thanks for the example! Added with a couple minor tweaks

  • shopt -s expand_aliases to make the aliases work in script
  • changed python -> python3 to be clearer

@Andrew7234 Andrew7234 force-pushed the andrew7234/cache-node-errors branch from a2ab5dc to 8fa0f8f Compare September 15, 2023 19:41
@Andrew7234 Andrew7234 changed the title Andrew7234/item analyzer regression tests testing: extend regression tests to item analyzers Sep 15, 2023
Base automatically changed from andrew7234/cache-node-errors to main September 15, 2023 19:47
@Andrew7234 Andrew7234 force-pushed the andrew7234/item-analyzer-regression-tests branch from fc36464 to ded19c7 Compare September 15, 2023 20:49
@mitjat
Copy link
Contributor

mitjat commented Sep 23, 2023

I pushed two three small commits:

  • The first one regenerates the RPC cache. The value found in the old cache was {"Result": null, "DeterministicErr": {}}, which is incompatible with the new type now that we renamed Result to Ok. A little wild that the subsequent attempt to actually call EVMSimulateCall caused a segfault 🤨, but I guess that's a worry for another day?
  • The second one is a tiny bugfix that I have in a branch of mine, but fits into this PR much better. When running e2e tests on this PR, I noticed that it affects it also.
  • (Ooops, another commit to update the expected e2e outputs)

@Andrew7234 Andrew7234 force-pushed the andrew7234/item-analyzer-regression-tests branch 2 times, most recently from 96e1842 to 79bf4b5 Compare September 23, 2023 09:52
stable sort

update regression tests

tmp

regenerate rpc cache

api: /consensus/entities/{id}: Stabilize output ordering

update regression outputs

test python3 ci

nits

nit
@Andrew7234 Andrew7234 force-pushed the andrew7234/item-analyzer-regression-tests branch from 79bf4b5 to a70d004 Compare September 23, 2023 09:58
@Andrew7234 Andrew7234 merged commit ba51dd9 into main Sep 23, 2023
@Andrew7234 Andrew7234 deleted the andrew7234/item-analyzer-regression-tests branch September 23, 2023 10:04
@csillag csillag mentioned this pull request Dec 30, 2024
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.

2 participants