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

Review excluded tests for long running times #1262

Closed
1 task done
kevaundray opened this issue May 1, 2023 · 5 comments
Closed
1 task done

Review excluded tests for long running times #1262

kevaundray opened this issue May 1, 2023 · 5 comments
Assignees
Labels
bug Something isn't working enhancement New feature or request

Comments

@kevaundray
Copy link
Contributor

Problem

We currently are excluding poseidon and sha2_byte in our tests because they were taking a long time to run.

The poseidon rationale can be seen here: #768 (comment)

This is non optimal because these tests are never ran.

Proposed solution

Review the CI times of these functions when no longer excluded. The poseidon test seems to add an extra 30 minutes to CI times which is not feasible, though as noted in the issue, it was contigent on a certain PR, so this seems to be a compiler problem.

The sha2_byte issue has a lot more unknowns and will require further investigation.

Alternatives considered

No response

Additional context

No response

Submission Checklist

  • Once I hit submit, I will assign this issue to the Project Board with the appropriate tags.
@kevaundray kevaundray added bug Something isn't working enhancement New feature or request labels May 1, 2023
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Noir May 1, 2023
@TomAFrench
Copy link
Member

TomAFrench commented May 1, 2023

Outside of these functions living in the standard library is there any reason to include them in the integration tests for the noir language itself? One benefit of #1258 is that we can offload these tests into external repositories to run separately.

Granted we had a bug which surfaced in the poseidon integration test however we have multiple versions of more minimal circuits which would work as regression tests.

@kevaundray
Copy link
Contributor Author

Outside of these functions living in the standard library is there any reason to include them in the integration tests for the noir language itself? One benefit of #1258 is that we can offload these tests into external repositories to run separately.

Granted we had a bug which surfaced in the poseidon integration test however we have multiple versions of more minimal circuits which would work as regression tests.

Good point -- for Poseidon I don't think there is any reason if we go with a thin stdlib, if we remove black box functions too, we also can remove the sha2 tests

@kevaundray
Copy link
Contributor Author

We should put in some time to figure out why they are so long running though -- as adoption increases we want folks to run CI on large/complicated programs

@kevaundray
Copy link
Contributor Author

@guipublic can you check if we are exlcuding these tests in the case of the new ssa and whether they are still long running?

@kevaundray kevaundray removed their assignment Jul 25, 2023
@kevaundray
Copy link
Contributor Author

Closing this because we no longer exclude any tests

@github-project-automation github-project-automation bot moved this from 📋 Backlog to ✅ Done in Noir Jul 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
Archived in project
Development

No branches or pull requests

3 participants