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

Allow re-use of chain reader tests outside go testing to allow applications to test against actual chains and optomize the use of the contracts. #13280

Merged
merged 4 commits into from
Jun 12, 2024
Merged

Allow re-use of chain reader tests outside go testing to allow applications to test against actual chains and optomize the use of the contracts. #13280

merged 4 commits into from
Jun 12, 2024

Conversation

nolag
Copy link
Contributor

@nolag nolag commented May 21, 2024

No description provided.

@nolag nolag requested review from a team as code owners May 21, 2024 17:12
@github-actions GitHub Actions
Copy link
Contributor

I see you updated files related to core. Please run pnpm changeset in the root directory to add a changeset as well as in the text include at least one of the following tags:

  • #added For any new functionality added.
  • #breaking_change For any functionality that requires manual action for the node to boot.
  • #bugfix For bug fixes.
  • #changed For any change to the existing functionality.
  • #db_update For any feature that introduces updates to database schema.
  • #deprecation_notice For any upcoming deprecation functionality.
  • #internal For changesets that need to be excluded from the final changelog.
  • #nops For any feature that is NOP facing and needs to be in the official Release Notes for the release.
  • #removed For any functionality/config that is removed.
  • #updated For any functionality that is updated.
  • #wip For any change that is not ready yet and external communication about it should be held off till it is feature complete.

@nolag nolag force-pushed the rtinianov_newtestableChain branch from e5bdae7 to b52a16f Compare May 21, 2024 17:21
@nolag nolag force-pushed the rtinianov_newtestableChain branch from b52a16f to 48d9e5b Compare May 21, 2024 17:50
@nolag nolag force-pushed the rtinianov_newtestableChain branch from 48d9e5b to d3ecbda Compare May 21, 2024 17:51
nolag
nolag commented May 21, 2024
View reviewed changes
core/services/relay/evm/evmtesting/chain_reader_interface_tester.go
db := it.Helper.NewSqlxDB(t)
lpOpts := logpoller.Opts{
PollPeriod: time.Millisecond,
FinalityDepth: 4,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I left this the same as it was before I did refactoring figuring that I could make tests run in a reasonable time. Would it make more sense for me to make this configurable so that things like finality breaches could be caught by the test? Does anyone know the impact of waiting for finality? For example, how long would it take to deploy a contract then send four transactions and wait for their events on Sepolia?

Copy link
Contributor

Choose a reason for hiding this comment

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

If we are only testing the ChainReader interface, I don't think finality matters so much as returning valid structured data. While the ChainReader interface tests are sort of integration tests with the underlying log poller, adjusting finality here seems to just duplicate logpoller tests. Maybe I'm missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, that's why I left it, I just wanted to present the the option and where it could benefit us. I'm not sure there would be much gain anyways, because finality breaches aren't constant anyway. So the odds you get one during a test seem close to zero.

@cl-sonarqube-production
Copy link

Quality Gate passed Quality Gate passed

Issues
2 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
99.2% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

EasterTheBunny
EasterTheBunny previously approved these changes May 21, 2024
View reviewed changes
Copy link
Contributor

@EasterTheBunny EasterTheBunny left a comment

Choose a reason for hiding this comment

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

LGTM

core/services/relay/evm/evmtesting/chain_reader_interface_tester.go
db := it.Helper.NewSqlxDB(t)
lpOpts := logpoller.Opts{
PollPeriod: time.Millisecond,
FinalityDepth: 4,
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are only testing the ChainReader interface, I don't think finality matters so much as returning valid structured data. While the ChainReader interface tests are sort of integration tests with the underlying log poller, adjusting finality here seems to just duplicate logpoller tests. Maybe I'm missing something?

@nolag nolag force-pushed the rtinianov_newtestableChain branch 2 times, most recently from 8c8e299 to 36cab61 Compare June 11, 2024 19:38
@nolag nolag force-pushed the rtinianov_newtestableChain branch from 36cab61 to 2e6d8fd Compare June 11, 2024 19:55
@nolag nolag requested review from jmank88 and EasterTheBunny June 11, 2024 19:55
EasterTheBunny
EasterTheBunny previously approved these changes Jun 11, 2024
View reviewed changes
@EasterTheBunny EasterTheBunny enabled auto-merge June 11, 2024 20:04
jmank88
jmank88 previously approved these changes Jun 11, 2024
View reviewed changes
@nolag nolag dismissed stale reviews from jmank88 and EasterTheBunny via e23f756 June 12, 2024 15:11
@nolag nolag force-pushed the rtinianov_newtestableChain branch from 2e6d8fd to e23f756 Compare June 12, 2024 15:11
jmank88
jmank88 previously approved these changes Jun 12, 2024
View reviewed changes
@nolag
Allow re-use of chain reader tests outside go testing to allow applic…
3d721fc 
…ations to test against actual chains and optomize the use of the contracts.
@nolag nolag force-pushed the rtinianov_newtestableChain branch from e23f756 to 3d721fc Compare June 12, 2024 15:24
@EasterTheBunny EasterTheBunny added this pull request to the merge queue Jun 12, 2024
Merged via the queue into develop with commit e27c6f5 Jun 12, 2024
105 of 106 checks passed
@EasterTheBunny EasterTheBunny deleted the rtinianov_newtestableChain branch June 12, 2024 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bolekk bolekk bolekk approved these changes

@jmank88 jmank88 jmank88 approved these changes

@EasterTheBunny EasterTheBunny EasterTheBunny left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nolag @jmank88 @bolekk @EasterTheBunny