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_resources.py into test_resources package #1672

Merged

Conversation

bvedang
Copy link
Member

@bvedang bvedang commented Nov 1, 2023

Problem

What problem have you solved?
The test_resources.py file had become large and cluttered with numerous classes, making it difficult to navigate, maintain, and understand the scope of individual tests. This complexity could potentially hinder future development and decrease overall code quality.

Solution
To address the issue, I refactored the test_resources.py file by organizing its contents into a more structured package named test_resources. This involved logically grouping related classes into separate module files within the package.

How did you solve the problem?
This involved logically grouping related classes into separate module files within the package.

The new structure is as follows:

basic_resources.py: Contains foundational resource classes.
enhanced_resources.py: Contains resource classes with enhanced features.
widgets.py: Hosts widget-related classes.
widget_tests.py: Contains tests specifically for widgets.
... [and so on for other modules]

Acceptance Criteria

Have you written tests? Have you included screenshots of your changes if applicable?
Did you document your changes?
No new tests were written as this was a refactoring task. But i made sure to run the make test command and check if all the test are passing

Copy link
Contributor

@matthewhegarty matthewhegarty left a comment

Choose a reason for hiding this comment

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

Thanks for working on this.

When I run make test in release-4 I get 475 tests run.
In this branch, I get 306, so some tests are being missed. This is probably because some of the new modules with tests in are not named 'test_*'. If you rename these modules it should work.

Feel free to update the changelog, and also add your name to AUTHORS if you like

.DS_Store Outdated Show resolved Hide resolved
@matthewhegarty
Copy link
Contributor

There is also a test failure due to a deprecation added to django. If you merge release-4 it should correct it

@bvedang
Copy link
Member Author

bvedang commented Nov 2, 2023

I have implemented the recommended changes from the review. The resources have been successfully extracted and restructured into tests. I've verified that the total number of tests remains consistent at 475, and all tests exhibit the same pass rate as before the refactor.

@matthewhegarty
Copy link
Contributor

Thanks. Am seeing one test failure in test_resources.py, the target string needs to be changed to:

"Field x does not exists in <class 'core.tests.test_resources.MyResource'> resource"

Copy link
Contributor

@matthewhegarty matthewhegarty left a comment

Choose a reason for hiding this comment

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

That's great. Thank you Vedang, much appreciated. Will merge.

@matthewhegarty matthewhegarty linked an issue Nov 3, 2023 that may be closed by this pull request
@matthewhegarty matthewhegarty merged commit e87a8df into django-import-export:release-4 Nov 3, 2023
11 checks passed
@bvedang
Copy link
Member Author

bvedang commented Nov 3, 2023

Hi Matthew,
I'm relatively new to the open-source community, and I just made my first contribution here.I'd love to get your feedback on how I could improve my contribution. Could you spare a few moments to take a look and suggest any enhancements?

Thank you for your guidance!

@matthewhegarty
Copy link
Contributor

Sure - no problem - I'll email you.

@bvedang bvedang deleted the refactor-test-resources branch January 2, 2024 01:23
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.

Refactor large test modules
2 participants