Fix flaky mongo devservices on windows

[evanchooly]

No description provided.

[evanchooly]

one of these days, i'll actually see these failures on a PR and catch them before main breaks. "but that is not this day!"

[gsmet]

Can you elaborate on what's going on here?
Because all these tests were passing before? Why do they suddenly fail? Or did we miss something before?

[evanchooly]

I fixed an issue to enable dev services on all named (and unnamed) mongodb client definitions. Seemed a simple enough fix but it's turned in a forest fire for no discernable reason.

[evanchooly]

ok to merge then @gsmet? I'd really like these failures to stop but when the PRs keep getting all greens it creates an annoying cycle. :)

[gsmet]

Frankly, I'm unhappy about it. You added a dev service feature and significantly reduced our code coverage on Windows for MongoDB in general. I don't see how that's acceptable?

We have dev services tests that are in a specific module. For instance: redis-devservices. That would allow you to test things without breaking everything.
But TBH, I haven't understood how your work would suddently make things fail everywhere. I would have expected them to have default connection that would enable dev services anyway if it was enabled.

And you could disable dev services for the modules where we don't want it enabled via configuration (i.e. for all the other MongoDB modules).
And that includes the change you made here: https://github.com/quarkusio/quarkus/pull/26011/files#diff-035b738235c0bd33125ae45363854ca4291ecb6ce4cbf7c8344490bc505d90f6 .

So I would say either you can get this sorted out in a reasonable timeline or I'm going to revert this set of patches until you are able to make things work without reducing our coverage and we could then merge the change again.

Let me know what you prefer. You would be less pressured with the latter but you might not like it.

[evanchooly]

I'm closing in on what I think is the issue. But reproducing it locally has proven difficult and even CI runs have all been green for me. But I think I have it properly isolated now (and I'm moving the devservice check off to its own IT). We'll see how CI treats but it hasn't been very helpful to me with this so fingers crossed.

[n1hility]

I share the desire of a deterministic, reproducible, and reliable CI. IMO Disabling tests and working the bug and later reenabling is a good strategy keep that up. In some cases I agree it's better just to revert the change, in particular if we know the change is causing side effects in other areas, or if we are close to tagging and the chances of having a solution in time are low. On the other hand, If it's a contained problem being worked on by a component/extension lead, I think it's totally reasonable to do the former approach, particularly if they are committed to resolving the underlying problem. They are most likely the best equipped to assess whether the problem is real or whether it's just flaky tests triggered by a timing change. I really think we should defer to Justin on how to best proceed here in recovering the situation. Correct me if I am wrong Justin, but I suspect you would gladly revert the change if you determined it was about to be shipped and break a bunch of people?

[gsmet]

@n1hility well, my problem is that it was never mentioned that it was a temporary fix. And I'm not sure it was designed to be.

I have no problem to merge a temporary fix to fix CI. But it has to be clear. I won't accept the MongoDB tests to be fully disabled on Windows forever for the sake of adding a dev service feature.

But I think I will have a look at it myself because it has been too long already.

[gsmet]

I'm having a look at this one.

[gsmet]

Let's wait for CI to pass but I think it should be as simple as: #26649 so that we don't start a Docker container for the default connection in the newly added tests (given the default connection is not defined, one is created when dev services are enabled).
I'll report back here once CI has passed in the other PR.

[gsmet]

#26649 has been merged.

Let's close this one.

[evanchooly]

sigh i had this.

[gsmet]

Not sure what exactly you had but your PR introduced a lot more failures so it was nowhere near ready.
My PR fixed CI without changing anything to what you originally did. Which is what we originally wanted.

Now, if you want to introduce a new module to test MongoDB dev services specifically as I originally suggested, it's very welcome but please:

• don't adjust other modules as I don't think it's needed now - or let's discuss it before you do
• I don't think you need to add this module to the native testing as you are testing dev mode and we don't care about native

[evanchooly]

I have those tests reenabled with updated configurations where necessary. there was one odd failure in an unexpected place that i'm tracking (with fix like the one you pushed, ironically). I said i was working on. Please respect my effort and give me the time you offered. At a minimum, stop closing PRs without consulting with the requester.

[gsmet]

CI is fixed so you have all the time you want.

[gsmet]

And no, there was not "one odd failure": the initial problem was not fixed, and there were a ton of other failures. See the report here: #26590 (comment) .

So yes, I decided to get a quick fix in (it was just one line) given the initial issue was simple to fix and it was failing for ages now.

What you were working on here was unclear so sorry if I missed your intention and you wanted to go further than simply fixing the original issue.

[evanchooly]

i finally have the devservices test isolated and excluded and fixed the mongodb tests that had been relying on the default connection getting picked up by the mongodb test resources infra.

code updated

[evanchooly]

@geoand @gsmet is this good to merge? i think this is finally the full, correct fix but i'd rather not have the build break on me again.

[geoand]

I'll let Guillaume decide

[evanchooly]

final call ... ?