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

chore: switch integration tests to mirror CLI usage #1179

Merged
merged 3 commits into from
Jul 11, 2023

Conversation

TomAFrench
Copy link
Member

Related issue(s)

Resolves #1177 (comment)

Description

Summary of changes

This PR changes the integration tests to work using the CLI directly to ensure exact parity between this and real-world usage. i.e. rather than using prove_and_verify we actually use nargo prove and nargo verify.

As part of this I've done some general refactoring as this code is unnecessarily complex (3 layers of nesting to just iterate through test cases).

Dependency additions / changes

Test additions / changes

Checklist

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt with default settings.
  • I have linked this PR to the issue(s) that it resolves.
  • I have reviewed the changes on GitHub, line by line.
  • I have ensured all changes are covered in the description.

Documentation needs

  • This PR requires documentation updates when merged.

Additional context

@TomAFrench
Copy link
Member Author

@kevaundray This adds another ~10mins to the ubuntu run (up from 18mins)

@TomAFrench
Copy link
Member Author

I've separated all the readability improvements into #1180 so we can merge those even in the case we don't go ahead with this change. (plus it will highlight the substantive changes here more clearly)

@kevaundray
Copy link
Contributor

Ah thats horrible

@jfecher
Copy link
Contributor

jfecher commented Apr 19, 2023

What is the reasoning for the extra 10 minutes of runtime?
Honestly that seems like a large enough downgrade that I'm wary of merging this at all

@TomAFrench
Copy link
Member Author

Let's park this pending #1180

@TomAFrench TomAFrench marked this pull request as draft April 19, 2023 16:56
@TomAFrench TomAFrench force-pushed the rework-integration-tests branch 2 times, most recently from b38221e to 97e5bb2 Compare April 19, 2023 23:48
@TomAFrench TomAFrench changed the title feat: switch integration tests to mirror CLI usage chore: switch integration tests to mirror CLI usage Jul 11, 2023
@TomAFrench TomAFrench marked this pull request as ready for review July 11, 2023 05:14
@TomAFrench
Copy link
Member Author

Comparing this PR with master.

There's a 5 second difference between the two now that we're not proving and verifying.

Copy link
Contributor

@jfecher jfecher left a comment

Choose a reason for hiding this comment

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

👍

@jfecher jfecher added this pull request to the merge queue Jul 11, 2023
Merged via the queue into master with commit 0d85752 Jul 11, 2023
@jfecher jfecher deleted the rework-integration-tests branch July 11, 2023 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants