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

Unify SQL query generation for sql-based providers. #147

Merged
merged 15 commits into from
Sep 29, 2024

Conversation

mo-esmp
Copy link
Member

@mo-esmp mo-esmp commented Sep 23, 2024

No description provided.

@mo-esmp mo-esmp requested a review from followynne September 23, 2024 21:22
@mo-esmp
Copy link
Member Author

mo-esmp commented Sep 24, 2024

@followynne A question: Is it necessary to run all tests against three target frameworks? It seems we are also testing the .NET Framework.

@followynne
Copy link
Member

@mo-esmp

I think it's a good practice and doesn't add too much overhead!

When I run tests, they run against NET 6/7/8 which are the supported and configured frameworks.
Do you see an issue with this configuration?

(btw, don't worry about MSSQL container test failing, it seems to be related to a broken latest docker image from MS - if you have a cached docker image locally, they work fine)

@mo-esmp
Copy link
Member Author

mo-esmp commented Sep 24, 2024

Yes, I have issues. Tests are slow and it takes too much time to be finished. I don't know why MS SQL tests are broken on CI and my local tests are OK. I tried to run tests on another laptop but got timeout error.

image

It seems I broke something, but I can't figure it out.

image

@followynne
Copy link
Member

@mo-esmp

approved!

about the tests, I'll answer you more in detail:

  • tests are slow locally 'cause they're integration tests and the Docker part is cool but... has a performance overheader that works fine on CI, locally it's probably heavy on laptop sources. That said, I still think it's not that bad when in CI and I'd keep the current setup... locally it's better to run a single provider per time and it gets better...
  • Elastic is an incredible pain to setup as integration - TestContainers works horribly with NEST package. Elastic has a new package for Elastic v8 but it requires a new provider, as of now. The integration tests are based on a package from Elastic, which is not great in terms of performance etc. Locally, if you get inconclusive results, you need to rerun the suite alone and it should go. I invested many hours/days searching a solution but I gave up improving it, since it still works on CI
  • MS SQL issue: tests/Serilog.Ui.MsSqlServerProvider.Tests/Util/MsSqlServerTestProvider.cs, I added this reference [Bug]: MsSql health check does not complete on newest container image testcontainers/testcontainers-dotnet#1220 (comment). There's an issue with a latest image and you're hitting it right now... if you fix in tests the image to a previous mssql-2022 version, it will works. On my local I have this cached version and it works...
  • all other tests work fine, I checked the CI and they go all green!

@followynne followynne merged commit ea83b81 into master Sep 29, 2024
7 checks passed
@followynne followynne deleted the feature/refactor-sql-query-generation branch September 29, 2024 21:44
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