Skip to content

Heap invariant testing #68

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
Nov 29, 2022
Merged

Heap invariant testing #68

merged 10 commits into from
Nov 29, 2022

Conversation

@MathisGD
Copy link
Collaborator

@MathisGD MathisGD commented Sep 22, 2022 •
edited
Loading

This PR adds invariant testing for the HeapOrdering. This is particularly suited for data structures.

Questions:

  • Should I remove the old TestRandomHeam ? (my answer would be yes)
  • Should I put them in the same file than the other tests (I would also say yes)

To do:

  • Basic invariants two-heap
  • Basic invariants three-heap
  • Test max sorted users

@MathisGD MathisGD self-assigned this Sep 22, 2022
@MathisGD MathisGD changed the base branch from main to feat/three-heap September 25, 2022 19:45
@MathisGD MathisGD requested a review from QGarchery September 25, 2022 20:30
@MathisGD MathisGD force-pushed the test/heap-invariant branch 3 times, most recently from db4a55d to dff8ec4 Compare October 13, 2022 21:27
@MathisGD MathisGD marked this pull request as ready for review October 13, 2022 21:35
@pakim249CAL
Copy link

pakim249CAL commented Oct 13, 2022

The test run seems quite long? We should reduce the number of runs somehow, or not include this in our general testing commands/CI.

@MathisGD
Copy link
Collaborator Author

MathisGD commented Oct 14, 2022

The test run seems quite long? We should reduce the number of runs somehow, or not include this in our general testing commands/CI.

Locally it was ok (~10min), but it seems that it is too heavy for the CI 😬

MerlinEgalite
MerlinEgalite approved these changes Oct 15, 2022
View reviewed changes
Copy link
Contributor

@MerlinEgalite MerlinEgalite left a comment

Choose a reason for hiding this comment

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

LGTM

Base automatically changed from feat/three-heap to main November 11, 2022 18:25
@MathisGD MathisGD force-pushed the test/heap-invariant branch from 17be1fa to f38e75f Compare November 27, 2022 10:21
MerlinEgalite
MerlinEgalite approved these changes Nov 27, 2022
View reviewed changes
Copy link
Contributor

@MerlinEgalite MerlinEgalite left a comment

Choose a reason for hiding this comment

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

LGTM

MerlinEgalite
MerlinEgalite approved these changes Nov 27, 2022
View reviewed changes
Copy link
Contributor

@MerlinEgalite MerlinEgalite left a comment

Choose a reason for hiding this comment

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

LGTM

QGarchery
QGarchery requested changes Nov 28, 2022
View reviewed changes
Copy link
Collaborator

@QGarchery QGarchery left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Awesome work ! A few things to change I think

@MathisGD MathisGD force-pushed the test/heap-invariant branch from bb1ffab to 9096ebd Compare November 28, 2022 16:18
@MathisGD MathisGD mentioned this pull request Nov 28, 2022
Closed
@MathisGD MathisGD force-pushed the test/heap-invariant branch from 9096ebd to e3409ac Compare November 28, 2022 16:20
@MathisGD MathisGD requested a review from QGarchery November 28, 2022 21:47
@MathisGD MathisGD merged commit d081cfb into main Nov 29, 2022
@MathisGD MathisGD deleted the test/heap-invariant branch November 29, 2022 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@QGarchery QGarchery QGarchery approved these changes

@MerlinEgalite MerlinEgalite MerlinEgalite approved these changes

@Rubilmax Rubilmax Awaiting requested review from Rubilmax

+1 more reviewer

@pakim249CAL pakim249CAL pakim249CAL approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@MathisGD MathisGD

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@MathisGD @pakim249CAL @QGarchery @MerlinEgalite