-
Notifications
You must be signed in to change notification settings - Fork 202
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
FreeQueueSAB: automated test coverage #374
Conversation
The PR at the moment only adds two packages. Perhaps it's not ready for the review yet? :) |
Hey, @hoch! Hope you are doing well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks generally good, but I assume this is the first PR of many. Right?
As per the discussion in the last meeting, we will cover all the test cases related to the FreeQueueSAB class in this PR and create a new PR for all test cases related to the FreeQueue class. I'll be committing more test cases related to performance testing soon. Please provide me a review and any case that is not covered according to you so that we can move towards finalizing this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm getting some errors when trying to run the tests. Please take a look.
Is it possible to include the necessary mocha and chai support files locally, or implement the parts we need separately?
Also we may need to include the following line in .eleventy.js to include the test html file in the project:
'src/lib/**/*.html',
…n, eleventry update, html bug fixes
I'm seeing the same failure in the "should fail to push data when buffer is full" test. I'll take a look too. |
I figured it out. I'll push the update tomorrow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall. If you agree with my other comments please make the changes and we can merge this.
I noticed that there is still the console error about the unexpected export token in chai.js, but it doesn't seem to cause any problems so I don't think we need to worry about it.
We are good to merge this PR now. |
Fixes #347