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

Refactorize tests to use Dataset as context manager #2191

Merged
merged 57 commits into from Apr 19, 2021

Conversation

albertvillanova
Copy link
Member

@albertvillanova albertvillanova commented Apr 8, 2021

Refactorize Dataset tests to use Dataset as context manager.

@albertvillanova albertvillanova added the refactoring Restructuring existing code without changing its external behavior label Apr 8, 2021
Copy link
Member

@lhoestq lhoestq left a comment

Choose a reason for hiding this comment

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

Thanks !

This makes things a bit harder to read sometimes but this is worth it since it makes the errors on windows way more readable.

Maybe in the future we could use pytest instead, and use a fixture that properly closes the datasets created during each test ?

@albertvillanova albertvillanova changed the title Factorize tests to use Dataset as context manager Refactorize tests to use Dataset as context manager Apr 9, 2021
@albertvillanova
Copy link
Member Author

I find very interesting that idea of using a fixture instead!

Let me rework a little bit this PR, @lhoestq.

@albertvillanova albertvillanova added this to the 1.6 milestone Apr 12, 2021
@albertvillanova
Copy link
Member Author

albertvillanova commented Apr 13, 2021

@lhoestq, as this is a big refactoring, I had many problems to solve the conflicts with the master branch...

Therefore, I think it is better to merge this as it is, and then to make other PRs with additional refactorings, before I get conflicts again with the master branch...

@lhoestq
Copy link
Member

lhoestq commented Apr 14, 2021

There are still some conflicts that prevent merging.
Moreover I noticed that you added one fixture per method of the Dataset object to be mocked. The code of all these fixtures is pretty much the same, feel free to factorize them into one fixture.

Also feel free to create another branch from master if you don't want to fix the conflicts of this branch.
Let me know if I can help you on this

@albertvillanova
Copy link
Member Author

albertvillanova commented Apr 14, 2021

@lhoestq, yes, the new conflicts appeared after today merge commits on master...

I am definitely going to split this PR into smaller ones in order to avoid having to resolve many conflicts after each commit on master. There are lots of conflicts and these are painful to resolve.

@albertvillanova albertvillanova merged commit c22ec7e into huggingface:master Apr 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Restructuring existing code without changing its external behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants