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

More shrinking! #39

Merged
merged 15 commits into from
Jul 10, 2021
Merged

More shrinking! #39

merged 15 commits into from
Jul 10, 2021

Conversation

Rewbert
Copy link
Collaborator

@Rewbert Rewbert commented Jul 7, 2021

Rewrote some of the shrinking strategies to reuse more code & shrink more stuff. There's also 2 bug-fixes to the runtime system here.

I split the shrinking up into several modules (one module per shrinking strategy). This, imo, makes it easier to get an overview of how each shrinking strategy is implemented. It helped me debug some of them while I was doing this.

@Rewbert Rewbert requested a review from j-hui July 7, 2021 13:44
Copy link
Collaborator

@j-hui j-hui left a comment

Choose a reason for hiding this comment

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

Awesome! Really looking forward to having truly minimal failing test cases (:

assert(sv->later_time != NO_EVENT_SCHEDULED);
sv->later_time = NO_EVENT_SCHEDULED;
dequeue_event(event_queue, &event_queue_len, idx);
if(sv->later_time != NO_EVENT_SCHEDULED) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Under what circumstances does this check become necessary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If you are trying to dequeue an event on a variable that has no future event scheduled for it, this function will look for it in the queue, not find the index of it and then ASSERT(QUEUE_HEAD <= idx) will fail.

Copy link
Collaborator Author

@Rewbert Rewbert Jul 9, 2021

Choose a reason for hiding this comment

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

I noticed this since the assertion would fail in a lot of the generated test programs, but because we did not inspect stderr to find out if execution succeeded well, we didn't notice it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

we shouldn't even be using these assertions in the first place, since as you observed, these don't convey the right information to the debug harness. We should replace these with our own assertion macros that print out messages the trace parser can understand.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I presume this check/assertion only fails when a process exits while updates are still scheduled for its local variables; we should add a comment to indicate that.

Given the invariant that all events not in the queue have later_time set to NO_EVENT_SCHEDULED, this is functionally equivalent to checking that the return value of index_of_event is non-zero, but a lot more efficient. We should make this clear in the comment.

runtime/src/ssm-sched.c Outdated Show resolved Hide resolved
test/lib/Test/SSM/Build.hs Show resolved Hide resolved
test/lib/Test/SSM/QuickCheck/Shrink/Procedures.hs Outdated Show resolved Hide resolved
@j-hui
Copy link
Collaborator

j-hui commented Jul 9, 2021

adding note that this fixes #31

@Rewbert
Copy link
Collaborator Author

Rewbert commented Jul 10, 2021

Awesome! Really looking forward to having truly minimal failing test cases (:

Fingers crossed that we'll get some nice, small test-cases now! Please let me know as soon as you find any test case which you see could have been shrunk more, and I'll try to add strategies to match these specific instances.

@Rewbert Rewbert merged commit a6242bf into master Jul 10, 2021
@j-hui j-hui mentioned this pull request Jul 17, 2021
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