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

Revisit and refactor/remove old e2e tests #7059

Closed
nventuro opened this issue Jun 14, 2024 · 0 comments · Fixed by #7621
Closed

Revisit and refactor/remove old e2e tests #7059

nventuro opened this issue Jun 14, 2024 · 0 comments · Fixed by #7621

Comments

@nventuro
Copy link
Contributor

There's multiple end to end tests that are either inefficient, outdated, , or simply superfluous due to behavior changing and these not keeping up. Examples include tests that assert conditions that have already been asserted in the library functions they're calling and supposedly testing.

We can use this issue to mark them as we run into them so that they can eventually be addressed.

AztecBot pushed a commit to AztecProtocol/aztec-nr that referenced this issue Jul 30, 2024
`get_notes` used to fail if no notes were found ever since
AztecProtocol/aztec-packages#4988 was merged.
The argument there was that since no constraints are applied, and note
non-existence cannot be proved, `get_notes` returning no notes is always
a mistake.

However, what that mistake means depends on the caller. Since we don't
want for the caller to have to pass an error message to `get_notes`, I
think it's better to return no notes and leave it up to the caller to
decide what to do. `BoundedVec` is a safe container: `get` will result
in errors if used with indices out of bounds.

I changed the callsites where we did `get_unchecked(0)` to `get(0)` -
this means that the errors now change from 'got 0 notes' to 'out of
bounds access'. Neither error message is great, so I didn't bother
improving this. Maybe we could have a `get` variant that received an
error message (like `Option::expect`), but we don't need to worry about
this for now.

Closes AztecProtocol/aztec-packages#7059
(indirectly due to how we need to change certain tests to address this)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

1 participant