Skip to content

add WillIterateCycle event#790

Merged
carljm merged 2 commits intosalsa-rs:masterfrom
carljm:cycle-event
Apr 10, 2025
Merged

add WillIterateCycle event#790
carljm merged 2 commits intosalsa-rs:masterfrom
carljm:cycle-event

Conversation

@carljm
Copy link
Contributor

@carljm carljm commented Apr 10, 2025

I would like to write tests asserting about cycles iterated and log cycles iterated as a metric. Add a WillIterateCycle event and fire it whenever a cycle head is about to iterate the cycle again. Update some cycle tests accordingly.

@carljm carljm requested a review from MichaReiser April 10, 2025 03:28
@netlify
Copy link

netlify bot commented Apr 10, 2025

Deploy Preview for salsa-rs canceled.

Name Link
🔨 Latest commit 05b8de1
🔍 Latest deploy log https://app.netlify.com/sites/salsa-rs/deploys/67f7c1c1a46cb20008fe3d04

@codspeed-hq
Copy link

codspeed-hq bot commented Apr 10, 2025

CodSpeed Performance Report

Merging #790 will not alter performance

Comparing carljm:cycle-event (05b8de1) with master (b165ba7)

Summary

✅ 12 untouched benchmarks

Copy link
Contributor

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

I think that's not just useful for third-party tests but also for our own. We don't have good infrastructure to set up tracing in tests, which is why I mainly use the db events to debug test errors (with occasional println sprinkled in every now and there)

Copy link
Contributor

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

I think that's not just useful for third-party tests but also for our own. We don't have good infrastructure to set up tracing in tests, which is why I mainly use the db events to debug test errors (with occasional println sprinkled in every now and there). In fact, I had to add multiple println expressions to verify that my test used the cycle head that I intended.

@carljm
Copy link
Contributor Author

carljm commented Apr 10, 2025

We don't have good infrastructure to set up tracing in tests

Side note from this PR, but if you replace #[test] with [#test_log::test] on a Salsa test and then run with RUST_LOG set, you get tracing output -- I use this a lot.

@MichaReiser
Copy link
Contributor

We don't have good infrastructure to set up tracing in tests

Side note from this PR, but if you replace #[test] with [#test_log::test] on a Salsa test and then run with RUST_LOG set, you get tracing output -- I use this a lot.

Ohhh nice, I didn't know about that.

@carljm carljm enabled auto-merge April 10, 2025 13:04
@carljm carljm added this pull request to the merge queue Apr 10, 2025
Merged via the queue into salsa-rs:master with commit 87bf6b6 Apr 10, 2025
11 checks passed
@carljm carljm deleted the cycle-event branch April 10, 2025 13:24
@github-actions github-actions bot mentioned this pull request Apr 9, 2025
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.

2 participants