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

Migrate dict.c unit tests to new test framework #673

Draft
wants to merge 2 commits into
base: unstable
Choose a base branch
from

Conversation

jay-tau
Copy link

@jay-tau jay-tau commented Jun 19, 2024

Fixes #428 for dict.c

Currently, all tests are in a single function. Before splititng it into induvidual unit tests, I wanted to ensure that everything was working as expected.

Currently, the 3rd test is raising a SIGSEV.
By performing print debugging, I have concluded that the issue lies in this line.
This type is declared in monotonic.h, so I am unsure why it is leading to a segfault.
Note, I believe it might be the same issue described by @jazelly here

Because of the issue with the monotime timer, all calls to dictRehashMicroseconds are leading to a segfault.

@madolson I would appreciate your thoughts. I am currently unsure how to proceed.
Please do let me know if I can provide any more information in this regard.

@jay-tau jay-tau marked this pull request as draft June 19, 2024 11:37
@flowerysong
Copy link
Contributor

This feels like it's probably a missing monotonicInit(), resulting in getMonotonicUs still being null and the call to elapsedStart() segfaulting.

monotonicInit(); /* Required for dict tests, that are relying on monotime during dict rehashing. */
is where this was called for the old test framework.

@jay-tau
Copy link
Author

jay-tau commented Jun 20, 2024

Thank you @flowerysong. Initialising monotonic as you suggested solved the issue :D

Now that this is fixed, I am able to think about how to structure the tests:

  • Would it be better to go ahead and break each test into a separate function? Because each test depends on the result of the previous ones right now.
  • Also, what would be the best way to handle running the benchmarks? Should they have an assert statement, to make sure that the time taken is within some expected value? I think this might not be viable considering the variety of hardware that the tests might run on.

Note: I have taken a look at some of the other unit testing PRs and didn't observe a clear pattern on when to split the tests into multiple functions. However, from a logical standpoint, I think it would make sense to have individual tests that do not rely on the TEST directive, rather than one single monolithing test.

Copy link

codecov bot commented Jun 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.23%. Comparing base (ae2d421) to head (17a40cf).
Report is 5 commits behind head on unstable.

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable     #673      +/-   ##
============================================
+ Coverage     70.03%   70.23%   +0.20%     
============================================
  Files           110      110              
  Lines         60076    60084       +8     
============================================
+ Hits          42076    42202     +126     
+ Misses        18000    17882     -118     
Files Coverage Δ
src/server.c 88.54% <ø> (ø)

... and 13 files with indirect coverage changes

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.

Migrate unit tests to new framework
2 participants