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.

Sign up for GitHub

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

Jump to bottom

Andrew7234/ci e2e #410

Merged
merged 1 commit into from May 19, 2023
Merged

Andrew7234/ci e2e #410

merged 1 commit into from May 19, 2023

Conversation

Andrew7234
Copy link
Collaborator

@Andrew7234 Andrew7234 commented May 5, 2023 •
edited

https://app.clickup.com/t/866a5pprn

Note: I'm holding off on making this step required in CI for a week or longer in order to verify that these tests are as reliable as we hope they are.

  • Timestamps are displayed differently in CI "2022-04-11T11:12:31Z" vs local machine "2022-04-11T04:12:31-07:00" [x]
  • Also this one:
index 35266bd..a49fe6d 100644
--- a/home/runner/work/oasis-indexer/oasis-indexer/tests/e2e_regression/expected/spec.headers
+++ b/home/runner/work/oasis-indexer/oasis-indexer/tests/e2e_regression/actual/spec.headers
@@ -1,7 +1,7 @@
 HTTP/1.1 200 OK
 Accept-Ranges: bytes
 Content-Length: UNINTERESTING
-Content-Type: text/plain; charset=utf-8
+Content-Type: application/x-yaml``` [x]

mitjat
mitjat reviewed May 8, 2023
View reviewed changes
Copy link
Collaborator

@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.

Looking good!

Looks like run.sh is giving false positives?

  • I added suggestions for easier debugging of CI failures (in this PR and in the future).
  • IIRC we might still have some queries that return results in a nondeterministic order; that could explain the failures.

tests/e2e_regression/run.sh Outdated Show resolved Hide resolved
.github/workflows/ci-test.yaml Show resolved Hide resolved
tests/e2e_regression/e2e_config.yml Outdated Show resolved Hide resolved
@Andrew7234 Andrew7234 force-pushed the andrew7234/ci-e2e branch 5 times, most recently from c8c1908 to 1da672c Compare May 16, 2023 20:47
@mitjat mitjat mentioned this pull request May 16, 2023
Merged
@Andrew7234 Andrew7234 force-pushed the andrew7234/ci-e2e branch 2 times, most recently from 368c335 to 9f40e62 Compare May 17, 2023 00:36
@Andrew7234 Andrew7234 self-assigned this May 17, 2023
@Andrew7234 Andrew7234 requested a review from mitjat May 17, 2023 00:50
Andrew7234
Andrew7234 commented May 17, 2023
View reviewed changes
tests/e2e/e2e_test.go Outdated Show resolved Hide resolved
Andrew7234
Andrew7234 commented May 17, 2023
View reviewed changes
analyzer/block/block.go Outdated Show resolved Hide resolved
mitjat
mitjat approved these changes May 19, 2023
View reviewed changes
Copy link
Collaborator

@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.

Thank you! I'm genuinely excited to try this out in the next PRs.

Q: I vaguely remembered (and IIRC you confirmed) that some API outputs have a non-deterministic order. But I don't see any changes in the server code that fixes this. So is everything deterministic after all?

tests/e2e_regression/e2e_config.yml
Comment on lines +13 to +16
# metadata_registry:
# interval: 5m
# aggregate_stats:
# tx_volume_interval: 5m
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note: Our non-block-based analyzers also have a fair bit of complexity in them, and it would be good to test them also. That's a separate task though.

The git-based metadata_registry is an exception, but everything else we could fit into this 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 aggregate_stats. We just don't have a way yet of telling them "exit when there is no more work for you".

Copy link
Collaborator

Choose a reason for hiding this comment

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

Created https://app.clickup.com/t/866aak668

@Andrew7234
update e2e config
1c8e4e6 
script tweaks for running locally on MacOS

add pogreb cache for 100 blocks of consensus/emerald

add expected api responses

add e2e_regression test to ci; add timeout to run.sh

add postgres step

remove extraneous -it from make postgres

test: extend psql delay

more sed -i wrangling

apply reviewer suggestions

upload indexer responses on failure

test

standardize timezone to UTC

standardize yaml Content-Type header

[analyzer] don't fetch latest block height; instead default to max uint

[ci] update rpc-cache

[ci] update spec for changes in 415

nit

test

test

address comments; add intra-batch delay on slow-sync

nit

nit

various api changes

nit

nit
@Andrew7234 Andrew7234 enabled auto-merge May 19, 2023 06:50
@Andrew7234 Andrew7234 merged commit 9ae106d into main May 19, 2023
6 checks passed
@Andrew7234 Andrew7234 deleted the andrew7234/ci-e2e branch May 19, 2023 14:59
@Andrew7234
Copy link
Collaborator Author

Q: I vaguely remembered (and IIRC you confirmed) that some API outputs have a non-deterministic order

Yeah originally I was seeing the /validator endpoint returning different orderings, but it became deterministic after rebasing last week. I assumed it was due to an outdated set of api responses at the time. I haven't seen any issues with it in 10+ runs since then; both locally and on CI, but it's one of the reasons why I'm holding off on making the check required in CI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mitjat mitjat mitjat approved these changes

@ptrus ptrus ptrus approved these changes

@aefhm aefhm Awaiting requested review from aefhm

@pro-wh pro-wh Awaiting requested review from pro-wh pro-wh is a code owner

Assignees

@Andrew7234 Andrew7234

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Andrew7234 @mitjat @ptrus