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

Modernize WatchServiceFileSystemWatcher and watch .env for dev mode #41317

Merged
merged 2 commits into from
Jul 11, 2024

Conversation

gsmet
Copy link
Member

@gsmet gsmet commented Jun 19, 2024

Fixes #41282

I can't say I'm a big fan of the fix but we will have to live with this for now.

@stuartwdouglas while working on this, I stumbled upon a small issue: if we are filtering tests, the counter of run tests (All 5 tests are passing (0 skipped)) is not updated when we add filters (even when we force a re-run).
My understanding is that we actually don't clear the test results but only push diffs to the test results (in TestState). I wonder if when we fully rerun the tests, we should actually clear the status but I'm not entirely sure, that's what you wanted?
The reproducer here is quite useful: https://github.com/flyinfish/quarkus--continuous-testing-dotenv . Run dev mode, then uncomment the filters in src/main/resources/application.properties and you should see the problem.

@radcortez at some point, we should probably think of a way to achieve this in a fully supported manner as this thing is quite brittle :). Not sure it's high priority though.

@radcortez
Copy link
Member

@radcortez at some point, we should probably think of a way to achieve this in a fully supported manner as this thing is quite brittle :). Not sure it's high priority though.

We could probably use

public static SmallRyeConfigBuilder emptyConfigBuilder() {
return new SmallRyeConfigBuilder()
.forClassLoader(Thread.currentThread().getContextClassLoader())
.withCustomizers(new QuarkusConfigBuilderCustomizer())
.addDiscoveredConverters()
.addDefaultInterceptors()
.addDiscoveredInterceptors()
.addDiscoveredSecretKeysHandlers()
.addDefaultSources()
.withSources(new ApplicationPropertiesConfigSourceLoader.InFileSystem())
.withSources(new ApplicationPropertiesConfigSourceLoader.InClassPath())
.withSources(new DotEnvConfigSourceProvider());
}
and retrieve any required configuration from that instance.

@gsmet gsmet force-pushed the TestSupport-dotenv branch 2 times, most recently from 9ba4adf to d58bfb5 Compare July 4, 2024 10:56
@gsmet
Copy link
Member Author

gsmet commented Jul 4, 2024

@radcortez so I went with your idea but went with an even more stripped version of SmallRyeConfig to avoid discovering the converters... could you have a look at the first commit?

@geoand I would appreciate a look at the second commit. I modernized WatchServiceFileSystemWatcher to use java.nio and more modern constructs and also added the ability to monitor a specific file (in this case, the .env).

@gsmet gsmet changed the title Include .env in the config hack of TestSupport Modernize WatchServiceFileSystemWatcher and watch .env for dev mode Jul 4, 2024
@geoand
Copy link
Contributor

geoand commented Jul 4, 2024

Seems reasonable, but CI seems to be complaining

@radcortez
Copy link
Member

@radcortez so I went with your idea but went with an even more stripped version of SmallRyeConfig to avoid discovering the converters... could you have a look at the first commit?

Perfect!

@quarkus-bot

This comment has been minimized.

@gsmet
Copy link
Member Author

gsmet commented Jul 9, 2024

Mkay. Doesn't look good. I will have a look at these particular tests...

Fixes quarkusio#41282

I can't say I'm a big fan of the fix but we will have to live with this
for now.
- Switch to java.nio consistently
- Allow to monitor a directory for specific files
- Monitor .env file
@gsmet gsmet added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jul 11, 2024
@quarkus-bot
Copy link

quarkus-bot bot commented Jul 11, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 9a71a13.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

@gsmet gsmet merged commit bf2e945 into quarkusio:main Jul 11, 2024
52 checks passed
@quarkus-bot quarkus-bot bot added kind/bugfix and removed triage/waiting-for-ci Ready to merge when CI successfully finishes labels Jul 11, 2024
@quarkus-bot quarkus-bot bot added this to the 3.13 - main milestone Jul 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

continous testing doesnt reload on changes in .env
3 participants