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_modelresources package #1733

Merged

Conversation

bvedang
Copy link
Member

@bvedang bvedang commented Jan 7, 2024

Problem

What problem have you solved?

The test_resources.py module exceeded 1.8k lines, becoming difficult to navigate and manage over time as more resource tests were added.

Solution

How did you solve the problem?

  • Added test_modelresources package

  • Split ModelResource tests into multiple classes.

Acceptance Criteria

Have you written tests? Have you included screenshots of your changes if applicable?
Did you document your changes?

Tests executed against test_resources package to confirm no change in overall coverage or results.
No UI or documentation changes were required for this strictly organizational refactor.

@bvedang bvedang changed the title Refactored test_resources.py into test_modelresources package #1700 Refactored test_resources.py into test_modelresources package Jan 7, 2024
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 doing this. It is much better to have smaller modules like you have added. I have made a couple of comments which I think would improve the PR.

@bvedang
Copy link
Member Author

bvedang commented Jan 9, 2024

Hi @matthewhegarty

I have removed the s from the class name in updated refactored test classes as per the instruction and also rename the files

@matthewhegarty
Copy link
Contributor

I have merged recent changes from release-4 and pushed to this branch (in order to resolve conflicts). You'll need to pull the branch if you want the recent changes.

@matthewhegarty matthewhegarty linked an issue Jan 10, 2024 that may be closed by this pull request
@bvedang
Copy link
Member Author

bvedang commented Jan 12, 2024

Sure, thank you for resolving the conflicts

@matthewhegarty
Copy link
Contributor

I merged recent changes from release-4 so you'll need to pull these.
I am hoping to get this completed asap - do you need me to help at all?

@matthewhegarty
Copy link
Contributor

I'm happy to merge this as it is. I would like to do so soon because it is blocking other work. I will hold off if you want to make more changes. Please can you let me know.

@bvedang
Copy link
Member Author

bvedang commented Jan 13, 2024

Hi @matthewhegarty , I was trying to figure out what do i need to take care of, i am confused. Can you help me with it
Thank you

@matthewhegarty
Copy link
Contributor

Hi - it's all good - I will merge. Thanks for all your help.

@matthewhegarty matthewhegarty merged commit 35d5272 into django-import-export:release-4 Jan 13, 2024
13 checks passed
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.

test_resources.py is too large and can be broken up
2 participants