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

Refactored test_tracer_analysis_service module with additional testcases #2181

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SakshiKekre
Copy link
Collaborator

@SakshiKekre SakshiKekre commented Feb 18, 2025

Refactored the module to run following test cases:

  1. Empty tracer
  2. Incorrect/Missing variable - Variable not present in tracer
  3. Extract tracer segment for root variable
  4. Extract tracer segment for nested
  5. Extract tracer segment for leaf variable

Possible future cases in consideration:

  1. Case sensitivity of variables?
  2. Spacing of value after the variable - additional spaces or no spaces in between?

Copy link

codecov bot commented Feb 18, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.62%. Comparing base (b201ce4) to head (36bfc34).
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2181      +/-   ##
==========================================
+ Coverage   75.48%   75.62%   +0.14%     
==========================================
  Files          78       78              
  Lines        2921     2938      +17     
  Branches      309      309              
==========================================
+ Hits         2205     2222      +17     
  Misses        651      651              
  Partials       65       65              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@anth-volk anth-volk left a comment

Choose a reason for hiding this comment

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

Hey @SakshiKekre, this is a really great foray into testing! Thanks for the work you've done here. I had a couple comments inline, but a couple more for you:

  1. I'd move this file to tests/unit/services/test_tracer_analysis_service.py, since this will become the new test for that service. We'll knit this and Zeel's, I think it is, together down the road
  2. I'd add the following test cases:
  • A variable name that isn't a string (we should make sure this raises an error)
  • A case where two variables have the same name for some reason (I think it should return the first one in the data, if I remember correctly - let me know if you run into problems here)
  • A case where one variable is a subset of another variable, but the longer one occurs first (e.g., "this_is_a_variable" and "this_is_a_variable_with_a_longer_name") - obviously we'll want to be sure we're returning the correct one
  • A case where the result is not of type list[str] - we'll want to raise an error

Thanks again! Looking forward to seeing more of this down the road.

@SakshiKekre
Copy link
Collaborator Author

Hey @anth-volk , thanks for your inputs. These are really great test case suggestions. I'm working on incorporating them in my tests and had some thoughts/questions:

  1. A variable name that isn't a string (we should make sure this raises an error)

I'm assuming here that you mean the target function should raise an error. If my assumption is correct, do we want the test case to fail if the target function does not 'raise' an error?
As of now, it simply returns an empty result if the variable is not found (or is a garbage value as in this context). Should I go ahead and modify the target function to do this input validation and raise an error?


  1. A case where two variables have the same name for some reason (I think it should return the first one in the data, if I remember correctly - let me know if you run into problems here)

This works as expected; target function returns the first occurrence of the variable.


  1. A case where one variable is a subset of another variable, but the longer one occurs first (e.g., "this_is_a_variable" and "this_is_a_variable_with_a_longer_name") - obviously we'll want to be sure we're returning the correct one

This one fails. Target function incorrectly returns the output associated with the longer variable if it occurs before the actual (expected) variable in the tracer. Might have to fix the regex in the target function.


  1. A case where the result is not of type list[str] - we'll want to raise an error

By 'result', do you mean the output from target function (parse_tracer_output) or the input tracer (which is also a mocked result as expected to be returned by get_tracer) to the target function?
If it's the former, I'm not sure how we could make the target function return an incorrect result. Also, this is implicitly being tested by all of our existing test cases anyway where they are asserting the result to be equal to a tracer of type list[str].
In case you mean the latter, the same question as in case 1 above applies.


Please let me know your thoughts on how I should proceed with 1, 3 and 4.

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