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

Read me prop tests #2177

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Read me prop tests #2177

wants to merge 4 commits into from

Conversation

bdemann
Copy link
Member

@bdemann bdemann commented Oct 18, 2024

These prop tests we are going to pass on for right now because they are sufficiently covered in the e2e tests

This was linked to issues Oct 18, 2024
Copy link
Member

Choose a reason for hiding this comment

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

Probably remove this as we don't already have property tests for timers

Copy link
Member Author

Choose a reason for hiding this comment

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

We should have a design discussion about what these property tests should look like

Copy link
Member Author

Choose a reason for hiding this comment

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

The more I think about it the more I think that it's good enough with tests/end_to_end/candid_rpc/class_syntax/timers/. The timers tests is a very robust little test. I really don't see how adding arbitrariness to these timers will be feasible, reasonable, or worth while (as per #2185 (comment))

So let's talk about it, if afterwards we still want to make prop tests for the timer functions then we can delete this file and merge this pr, otherwise we can leave it and merge this pr.

Copy link
Member

Choose a reason for hiding this comment

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

Analyze the current property test for this and decide if we already have enough coverage

Copy link
Member Author

Choose a reason for hiding this comment

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

I think so

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.

Timer inspect_message
2 participants