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(levm): ensure returned errors are correct #1225

Merged

Conversation

maximopalopoli
Copy link
Contributor

@maximopalopoli maximopalopoli commented Nov 21, 2024

Motivation

Currently in EF tests we don't verify that the error returned by levm matches with the expected, the goal is to verify it, and print an error if it does not match.

Description

This PR includes:

  • The logic of verifying the exception is the expected, in exception_is_expected()
  • The print of the error (or errors) that the EVM may throw in case the error does not match or in case it does not return an error and one is expected.

@maximopalopoli maximopalopoli added ef-tests Hive tests, execution-spec-tests levm Lambda EVM implementation labels Nov 21, 2024
@maximopalopoli maximopalopoli self-assigned this Nov 21, 2024
@maximopalopoli maximopalopoli marked this pull request as ready for review November 21, 2024 19:04
@maximopalopoli maximopalopoli requested a review from a team as a code owner November 21, 2024 19:04
Copy link
Contributor

@JereSalo JereSalo left a comment

Choose a reason for hiding this comment

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

In exception_is_expected() it would be good if all returned errors were implemented (except for the TYPE 3 FORK error).
Anyway, it looks good to me. I would have liked to test what happens if I change something in the matches! to see it breaking, but it looks like it works fine.
For merging don't forget to comment any added println! for debugging

Copy link
Contributor

@ilitteri ilitteri left a comment

Choose a reason for hiding this comment

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

LGTM. Left one nit.

cmd/ef_tests/levm/runner.rs Outdated Show resolved Hide resolved
@maximopalopoli maximopalopoli merged commit 55517d5 into levm/fixing_ef_tests_execution Nov 22, 2024
20 checks passed
@maximopalopoli maximopalopoli deleted the levm/feat/ensure-errors-are-right branch November 22, 2024 15:18
@maximopalopoli maximopalopoli restored the levm/feat/ensure-errors-are-right branch November 22, 2024 15:18
@maximopalopoli maximopalopoli deleted the levm/feat/ensure-errors-are-right branch November 22, 2024 15:18
@maximopalopoli maximopalopoli restored the levm/feat/ensure-errors-are-right branch December 10, 2024 18:50
github-merge-queue bot pushed a commit that referenced this pull request Dec 11, 2024
**Motivation**

This PR is a revival of an older PR that implemented this feature, and
merged the branch in another which PR was closed. This is an adaptation
of it to the current codebase.

**Description**

See #1225 for more details.

---------

Co-authored-by: JereSalo <jeresalo17@gmail.com>
Co-authored-by: Jeremías Salomón <48994069+JereSalo@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ef-tests Hive tests, execution-spec-tests levm Lambda EVM implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants