-
Notifications
You must be signed in to change notification settings - Fork 1
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
test: adding unit tests to separate branch #15
Conversation
Reviewer's Guide by SourceryThis pull request introduces unit tests for the TES and WES converters, moving them to a dedicated branch to ensure focused testing. Additionally, it updates the CI workflow and README to reflect the new project structure and naming conventions. File-Level Changes
Tips
|
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.
Hey @Karanjot786 - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 6 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 9 issues found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
@@ -25,24 +25,20 @@ jobs: | |||
|
|||
- name: Lint with Ruff | |||
run: | | |||
poetry run ruff check src/ tests/ | |||
poetry run ruff check crategen/ tests/ |
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.
suggestion (testing): Consider adding a specific file path for tests.
To ensure that only the relevant test files are checked, consider specifying the exact path to the test files instead of using a general 'tests/' directory.
poetry run ruff check crategen/ tests/ | |
poetry run ruff check crategen/ tests/specific_test_file.py |
inputs = tes_data.get("inputs", []) | ||
outputs = tes_data.get("outputs", []) | ||
creation_time = tes_data.get("creation_time", "") | ||
end_time = tes_data.get("logs", [{}])[0].get("end_time", "") # Corrected to fetch from logs |
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.
issue (bug_risk): Check for potential IndexError.
If 'logs' is an empty list, this line will raise an IndexError. Consider adding a check to ensure 'logs' is not empty before accessing the first element.
"@id": id, | ||
"name": name, | ||
"description": description, | ||
"instrument": executors[0].get("image", None) if executors else None, |
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.
issue (bug_risk): Check for potential IndexError.
If 'executors' is an empty list, this line will raise an IndexError. Consider adding a check to ensure 'executors' is not empty before accessing the first element.
"status": state, | ||
"startTime": convert_to_iso8601(start_time), | ||
"endTime": convert_to_iso8601(end_time), | ||
"result": [{"@id": output.get("location", ""), "name": output.get("name", "")} for output in outputs], |
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.
issue (bug_risk): Check for potential KeyError.
Ensure that 'outputs' is a list of dictionaries and that each dictionary contains the keys 'location' and 'name'. If not, this line may raise a KeyError.
except ValueError: | ||
continue | ||
# Handle incorrect format or other issues | ||
return None |
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.
suggestion: Consider logging incorrect timestamp formats.
Instead of silently returning None for incorrect timestamp formats, consider logging a warning or error message to help with debugging.
return None | |
import logging | |
logging.basicConfig(level=logging.WARNING) | |
# Handle incorrect format or other issues | |
logging.warning("Incorrect timestamp format encountered.") | |
return None |
result = self.converter.convert_to_wrroc(wes_data) | ||
self.assertEqual(result, expected_wrroc_data) | ||
|
||
def test_convert_from_wrroc(self): |
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.
suggestion (testing): Add tests for invalid data formats
It would be beneficial to add tests that check how the converter handles invalid data formats, such as incorrect date formats or unexpected data types.
def test_convert_from_wrroc(self): | |
def test_convert_from_wrroc_invalid_data(self): | |
invalid_wrroc_data = { | |
"@id": 123, # Invalid type, should be a string | |
"date": "invalid-date-format" # Invalid date format | |
} | |
with self.assertRaises(ValueError): | |
self.converter.convert_from_wrroc(invalid_wrroc_data) |
@@ -0,0 +1,62 @@ | |||
import unittest |
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.
suggestion (testing): Consider using pytest for more concise tests
Using pytest
can make the tests more concise and easier to read. For example, you can use fixtures for setup and teardown, and avoid boilerplate code.
import unittest | |
import pytest |
@@ -0,0 +1,58 @@ | |||
import unittest |
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.
suggestion (testing): Consider using pytest for more concise tests
Using pytest
can make the tests more concise and easier to read. For example, you can use fixtures for setup and teardown, and avoid boilerplate code.
import unittest | |
import pytest |
} | ||
|
||
result = self.converter.convert_to_wrroc(tes_data) | ||
self.assertEqual(result, expected_wrroc_data) |
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.
suggestion (testing): Add assertion messages for better debugging
Adding custom messages to assertions can help with debugging when a test fails. For example: self.assertEqual(result, expected_wrroc_data, 'Conversion to WRROC failed')
.
self.assertEqual(result, expected_wrroc_data) | |
self.assertEqual(result, expected_wrroc_data, 'Conversion to WRROC failed') |
} | ||
|
||
result = self.converter.convert_to_wrroc(wes_data) | ||
self.assertEqual(result, expected_wrroc_data) |
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.
suggestion (testing): Add assertion messages for better debugging
Adding custom messages to assertions can help with debugging when a test fails. For example: self.assertEqual(result, expected_wrroc_data, 'Conversion to WRROC failed')
.
self.assertEqual(result, expected_wrroc_data) | |
self.assertEqual(result, expected_wrroc_data, 'Conversion to WRROC failed') |
Description:
This PR separates the unit tests from the main implementation and moves them to a dedicated branch. This is done to ensure focused testing and validation of individual components without merging the tests prematurely.
Files Included:
tests/unit/test_tes_converter.py
tests/unit/test_wes_converter.py
Details:
Unit Tests for TES Converter:
Unit Tests for WES Converter:
Instructions for Running the Tests:
Ensure all dependencies are installed:
Run the unit tests:
Sample TES Input (
tes_example.json
):Sample WES Input (
wes_example.json
):Summary by Sourcery
This pull request introduces new conversion functionalities for TES and WES formats to WRROC and vice versa. It also includes a CLI for performing these conversions, updates the project name to 'CrateGen', and adjusts the CI configuration accordingly. Additionally, unit tests for the new conversion functionalities have been added.