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

Unit test with testcontainers #405

Merged
merged 11 commits into from
Dec 17, 2023

Conversation

hoangthanh28
Copy link
Contributor

@hoangthanh28 hoangthanh28 commented Dec 11, 2023

Hi Erik.
I plan to contribute to grate because I love it. It's a great tool, please consider my PR with the following changes:

  • Refactoring the current unit test with testcontainers
  • Migrate the current NUnit to xUnit (I believe xUnit will be better for us in the long run)
  • Speedup the SqlServer Unit test, the duration is reduced from 21mins to 2mins
  • Adjust the code format using dotnet format (I'm using Debian 12, vs code and C# DevKit). Please let me know what the best option is for us to follow in terms of code formatting. Feel free to prune/reformat the code if needed. Sorry for the inconsistent style with yours.

@hoangthanh28 hoangthanh28 marked this pull request as ready for review December 11, 2023 02:24
@hoangthanh28 hoangthanh28 changed the title Features/thanh/testcontainer Unit test with testcontainers Dec 11, 2023
@erikbra
Copy link
Owner

erikbra commented Dec 11, 2023

Hi, @hoangthanh28. Thanks a lot for your PR! Great that you love great (pun intended), and even better that you would like to contribute! This is what open source should be, collaboration! :)

I originally used NUnit for grate, partly because it was used in RoundhousE, and partly because the "run this thing once before all tests" was easier in NUnit, at least I thought so at the time. So I used that for setting up the containers before running the tests. But I like xUnit better personally as well.

Your PR is quite big, so I'll have to take a little while looking into it, but initially I'm positive to the initiatives!

@hoangthanh28
Copy link
Contributor Author

Thank you Erik for your consideration. Btw, I'm a 5-year RoundHousE user, as well as a big fan of database first, changing database is crucial, so we need to script it down and put into the version control system. :)

@erikbra
Copy link
Owner

erikbra commented Dec 17, 2023

Thanks again, @hoangthanh28 - I have looked through it, and it looks very clean and good quality code, I like it!
In general, I would prefer having only one type of change in each PR, it's easier to see what is changed (e.g. changing to TestContainers, or changing to xUnit), but I understand that they might walk a bit hand-in-hand, as the dependency injection might have changed.

I'm going to merge this as-is. I have a few suggestions for improvements, but I'll just make a separate PR myself into main for them.

@erikbra erikbra merged commit e439dee into erikbra:main Dec 17, 2023
10 checks passed
@hoangthanh28 hoangthanh28 deleted the features/thanh/testcontainer branch December 18, 2023 01:50
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.

2 participants