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: Add lurk shard test #265

Closed
wants to merge 3 commits into from
Closed

chore: Add lurk shard test #265

wants to merge 3 commits into from

Conversation

wwared
Copy link
Contributor

@wwared wwared commented Aug 29, 2024

Fixes #259

Also addresses the nit from https://github.com/argumentcomputer/loam/pull/260#discussion_r1735229332

This PR does not try to generate a CryptoProof from the MachineProof since the former is in a private module, and exposing it might expose too much as pub we might not want to expose yet. It's extra annoying since tests/fib.rs is technically a separate crate so it couldn't just be pub(crate).

I've manually tested the conversion in the REPL, and indeed when there are multiple shards, all the shards have the same public values and the CryptoProof conversion works fine.

@arthurpaulino
Copy link
Member

This single test has increased the test time from 3m to 8m. I believe it's not running the tests in release mode.
Maybe it's worth changing that at this point

@wwared
Copy link
Contributor Author

wwared commented Aug 29, 2024

@arthurpaulino It's using the dev-ci profile which already has optimizations turned on. It's possible that running the tests in parallel made them slower since all cores are used. I tried changing the nextest config so it won't run the e2e and shard tests in parallel

@wwared
Copy link
Contributor Author

wwared commented Aug 29, 2024

Looks like it's still really slow in CI (~355s) for some reason, that didn't seem to help :/

For comparison, locally for me this test takes ~135s, which I figured was much more reasonable.

I'll mark this PR as draft for now, since I don't think more than doubling our CI wait times is worth it for this single particular test. Specially considering this test isn't exercising the full CryptoProof/IOProof path either

@wwared wwared marked this pull request as draft August 29, 2024 15:19
@arthurpaulino
Copy link
Member

For clarity, I'm not exactly against cost increases in our CI. If we need something, we need it.
I mentioned the above just so we make the decision consciously, without letting it slip through our eyes.

@wwared
Copy link
Contributor Author

wwared commented Sep 25, 2024

Closing for now -- will revisit later

@wwared wwared closed this Sep 25, 2024
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.

Add test with >1 shards for lurk proof
2 participants