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

FreeQueue: automated test coverage #388

Merged
merged 4 commits into from
Aug 14, 2024
Merged

FreeQueue: automated test coverage #388

merged 4 commits into from
Aug 14, 2024

Conversation

professorabhay
Copy link
Collaborator

@professorabhay professorabhay commented Jul 23, 2024

  • Test cases for FreeQueue Class

Fixes #347

@professorabhay professorabhay added the gsoc24 Google Summer of Code 2024 label Jul 23, 2024
@professorabhay professorabhay self-assigned this Jul 23, 2024
@professorabhay professorabhay changed the title Added Test Cases For FreeQueue Class FreeQueue: automated test coverage Jul 23, 2024
@professorabhay
Copy link
Collaborator Author

image

Copy link
Member

@hoch hoch left a comment

Choose a reason for hiding this comment

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

Generally looks great! Mostly nits.

src/lib/free-queue/free-queue.js Outdated Show resolved Hide resolved
src/lib/free-queue/free-queue.js Outdated Show resolved Hide resolved
src/lib/free-queue/test/free-queue.test.html Outdated Show resolved Hide resolved
src/lib/free-queue/test/free-queue.test.html Outdated Show resolved Hide resolved
src/lib/free-queue/test/free-queue.test.html Outdated Show resolved Hide resolved
src/lib/free-queue/test/free-queue.test.js Outdated Show resolved Hide resolved
src/lib/free-queue/test/free-queue.test.js Outdated Show resolved Hide resolved
src/lib/free-queue/test/free-queue.test.js Outdated Show resolved Hide resolved
src/lib/free-queue/test/free-queue.test.js Outdated Show resolved Hide resolved
src/lib/free-queue/test/free-queue.test.js Outdated Show resolved Hide resolved
@mjwilson-google
Copy link
Collaborator

Let's merge #374 first, then make sure this class has the same amount of coverage.

Copy link
Collaborator

@mjwilson-google mjwilson-google left a comment

Choose a reason for hiding this comment

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

Now that #374 is landed, let's take the best aspects of both PRs and apply them to both test suites.

src/lib/free-queue/test/free-queue.test.js Outdated Show resolved Hide resolved
src/lib/free-queue/test/free-queue.test.js Outdated Show resolved Hide resolved
src/lib/free-queue/test/free-queue.test.js Show resolved Hide resolved
src/lib/free-queue/test/free-queue.test.js Show resolved Hide resolved
src/lib/free-queue/test/free-queue.test.js Outdated Show resolved Hide resolved
src/lib/free-queue/test/free-queue.test.js Show resolved Hide resolved
Copy link
Collaborator

@mjwilson-google mjwilson-google left a comment

Choose a reason for hiding this comment

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

Looks good to me! Let's follow-up with another PR later to make both test suites more similar.

When I ran this., FreeQueue was significantly faster than FreeQueueSAB (about 2x). Is that what you were seeing too? We have to be a bit careful about reading too much into the results because this isn't a very accurate timing method though.

Please merge when you are ready.

@professorabhay
Copy link
Collaborator Author

professorabhay commented Aug 12, 2024

Yeah Sure!

Hmm, I am not sure but yes there was some difference. I'll look into that.

Sure, we can merge this PR and I'll raise a new PR for extending the SAB TestCases and resolve some nits.
I think I'm not authorized to merge the PR. So, you have to do that 😅

@mjwilson-google mjwilson-google merged commit 5d579b8 into main Aug 14, 2024
5 checks passed
@mjwilson-google
Copy link
Collaborator

Sorry, didn't realize you weren't authorized!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gsoc24 Google Summer of Code 2024
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve RingBuffer
3 participants