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

RawDataRecord with unit tests #297

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

n135c10r
Copy link
Collaborator

Description

Draft of Implementation for RawDataRecord with unit tests.
Refactoring the Data Records directory structure.

How Has This Been Tested?

Executing unit tests.

Process

I know the process and did my best to follow it

@n135c10r n135c10r added this to the 6) Database introduction milestone Nov 10, 2024
@n135c10r n135c10r added the implementation Changes to code label Nov 10, 2024
@n135c10r n135c10r changed the title RawDataRecord with unit tests [Draft] RawDataRecord with unit tests Nov 10, 2024
@mdabrowski1990 mdabrowski1990 linked an issue Nov 11, 2024 that may be closed by this pull request
Copy link
Owner

@mdabrowski1990 mdabrowski1990 left a comment

Choose a reason for hiding this comment

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

I like that you separated data_record module.
I have left plenty of comments, but I think this is a really good start.

Copy link

codecov bot commented Nov 11, 2024

Codecov Report

Attention: Patch coverage is 95.91837% with 2 lines in your changes missing coverage. Please review.

Project coverage is 99.93%. Comparing base (c65988b) to head (d71291e).

Files with missing lines Patch % Lines
uds/database/data_record/raw_data_record.py 95.45% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##              main     #297      +/-   ##
===========================================
- Coverage   100.00%   99.93%   -0.07%     
===========================================
  Files           46       48       +2     
  Lines         2818     2859      +41     
  Branches       345      351       +6     
===========================================
+ Hits          2818     2857      +39     
- Misses           0        2       +2     
Flag Coverage Δ
integration-tests 80.79% <53.06%> (-0.54%) ⬇️
integration-tests-branch 75.83% <53.06%> (-0.47%) ⬇️
unit-tests 99.93% <95.91%> (-0.07%) ⬇️
unit-tests-branch 99.86% <91.83%> (-0.14%) ⬇️
Files with missing lines Coverage Δ
uds/database/__init__.py 100.00% <100.00%> (ø)
uds/database/data_record/__init__.py 100.00% <100.00%> (ø)
uds/database/data_record/abstract_data_record.py 100.00% <100.00%> (ø)
uds/database/services/abstract_service.py 100.00% <100.00%> (ø)
uds/database/data_record/raw_data_record.py 95.45% <95.45%> (ø)

@n135c10r n135c10r changed the title [Draft] RawDataRecord with unit tests RawDataRecord with unit tests Nov 12, 2024
@n135c10r n135c10r marked this pull request as ready for review November 12, 2024 21:06
Copy link
Owner

Choose a reason for hiding this comment

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

I am going to help you with tests improvement.

self.__length = value

@property
def max_raw_value(self):
Copy link
Owner

Choose a reason for hiding this comment

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

let's move it to AbstractDataRecord (it is common for all DataRecords)

(8, 255),
]
)
def test_max_raw_value_getter(self, length, value):
Copy link
Owner

Choose a reason for hiding this comment

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

let's move it to AbstractDataRecord tests

you can test it without creating an object:
self.mock_data_record.length = length - to set length value
RawDataRecord.max_raw_value .fget(self.mock_data_record) - to get max_raw_value from property of RawDataRecord class

)
@patch(f"{SCRIPT_LOCATION}.isinstance")
def test_decode_type_error(self, mock_isinstance, value):
with pytest.raises(TypeError):
Copy link
Owner

@mdabrowski1990 mdabrowski1990 Nov 26, 2024

Choose a reason for hiding this comment

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

we need one more line before with pytest.raises:
mock_isinstance.return_value = False to make sure that type check failed

)
@patch(f"{SCRIPT_LOCATION}.isinstance")
def test_encode_type_error(self, mock_isinstance, value):
with pytest.raises(TypeError):
Copy link
Owner

Choose a reason for hiding this comment

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

we need one more line before with pytest.raises:
mock_isinstance.return_value = False to make sure that type check failed


# encode

@pytest.mark.parametrize(
Copy link
Owner

Choose a reason for hiding this comment

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

let's parametrize both value and max_raw_value (like in test_encode_value_error) and perform Boundary Value Analysis:
good candidates that work well with values in test_encode_value_error

@pytest.mark.parametrize(
        "value, max_raw_value", [
            (0, 2),
            (3, 2),
            (63, 6),
    ]
)

Copy link
Owner

@mdabrowski1990 mdabrowski1990 left a comment

Choose a reason for hiding this comment

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

almost ready to be merged
please check why codecov reports missing coverage (there is something wrong with TypeError tests, probably it would be solved after my comments are applied)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
implementation Changes to code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce Raw Data Record (signal for message database)
2 participants