Skip to content
This repository has been archived by the owner on Apr 15, 2024. It is now read-only.

test: add e2e test for data commitment window change #491

Merged
merged 8 commits into from
Sep 27, 2023

Conversation

rach-id
Copy link
Member

@rach-id rach-id commented Sep 21, 2023

Overview

Closes #485

Checklist

  • New and updated code has appropriate documentation
  • New and updated code has new and/or updated testing
  • Required CI checks are passing
  • Visual proof for any user facing features like CLI or documentation updates
  • Linked issues closed with keywords

@rach-id rach-id added orchestrator orchestrator related relayer relayer related testing labels Sep 21, 2023
@rach-id rach-id self-assigned this Sep 21, 2023
@codecov-commenter
Copy link

codecov-commenter commented Sep 21, 2023

Codecov Report

Merging #491 (77289e1) into main (e0fe84d) will decrease coverage by 1.13%.
Report is 1 commits behind head on main.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #491      +/-   ##
==========================================
- Coverage   25.37%   24.24%   -1.13%     
==========================================
  Files          29       29              
  Lines        2834     2949     +115     
==========================================
- Hits          719      715       -4     
- Misses       2029     2147     +118     
- Partials       86       87       +1     
Files Changed Coverage Δ
e2e/qgb_network.go 0.00% <0.00%> (ø)

... and 1 file with indirect coverage changes

e2e/Dockerfile_e2e Outdated Show resolved Hide resolved
e2e/celestia-app/config.toml Show resolved Hide resolved
Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

this is good! but in general do you think testing this in an e2e test is something that we want to do forever? Like, could we test this in an integration or even a unit test? or is this already tested in the app? The reason being that debugging and maintaining e2e tests are more difficult than a unit or even an integration

@rach-id
Copy link
Member Author

rach-id commented Sep 22, 2023

@evan-forbes good point. It was actually per request from Mustapha. In fact, we're testing the window change in unit tests and also using the celestia node integration suite. However, we're not testing the whole behaviour of the bridge from orchestrators, to relayer, and the bridge contract. This test closes the loop.

The issue is that it takes a bit of time to finish, 7 minutes or so. So, it might make sense to disable it from running with the E2E suite and make it run only manually.

What do you think?

@evan-forbes
Copy link
Member

I see! it certainly is good to do a check. Assuming its already tested in the app, and we're just interested in testing this in a one off way on an IRL network then perhaps we can do that with a script?

@rach-id
Copy link
Member Author

rach-id commented Sep 23, 2023

@evan-forbes we could, or simply we can disable this test and keep it there for reference. What do you think?

@evan-forbes
Copy link
Member

yeah definitely no reason to delete it. I'll defer to you on how often we run it. Thanks for going through all the effort to make sure that it works

Copy link
Member

@adlerjohn adlerjohn left a comment

Choose a reason for hiding this comment

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

utACK

@rach-id
Copy link
Member Author

rach-id commented Sep 27, 2023

I will merge this PR and open an issue to turn this test into a script. Thanks guys for the reviews 👍

@rach-id rach-id merged commit 2f804d2 into celestiaorg:main Sep 27, 2023
11 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
orchestrator orchestrator related relayer relayer related testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Write an E2E test for data commitment window change
4 participants