Skip to content

Conversation

@qmonmert
Copy link
Contributor

@qmonmert qmonmert commented Jan 2, 2026

Fix #31133

@qmonmert qmonmert marked this pull request as ready for review January 2, 2026 21:45
Copy link
Member

@mshima mshima left a comment

Choose a reason for hiding this comment

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

See https://github.com/jhipster/generator-jhipster/actions/runs/20666629893/job/59344143827?pr=31863.
Backend test is taking 10 minutes. It's probably probing Kafka server.

@qmonmert qmonmert force-pushed the fix/31133v2 branch 3 times, most recently from 00577ff to 602cabc Compare January 3, 2026 15:47
@qmonmert qmonmert force-pushed the fix/31133v2 branch 5 times, most recently from 4186c1d to f3abf03 Compare January 10, 2026 20:55
@qmonmert qmonmert requested review from DanielFran and mshima January 12, 2026 09:48
@qmonmert
Copy link
Contributor Author

Before
image

After
image

@qmonmert
Copy link
Contributor Author

@mshima WDYT?

@mshima
Copy link
Member

mshima commented Jan 17, 2026

@ActiveProfiles("kafka") doesn’t work?
Instead of loading the file manually?

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR addresses issue #31133 by making Kafka test containers optional rather than required for all tests. The changes enable tests to use Spring Cloud Stream's in-memory TestChannelBinder by default, only requiring Kafka-specific configuration when explicitly opted in via the @EmbeddedKafka annotation.

Changes:

  • Removed automatic @EmbeddedKafka annotation from all integration tests
  • Modified test configuration to disable Kafka by default and use test binder
  • Created separate application-kafka.yml profile for Kafka-enabled tests
  • Updated context customizer to load Kafka config only when @EmbeddedKafka is present

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
KafkaTestContainersSpringContextCustomizerFactory.java.ejs Changed from starting Kafka containers to loading application-kafka.yml when @embeddedkafka is detected
generator.ts Removed automatic @embeddedkafka annotation addition to integration tests
generator.spec.ts.snap Updated snapshot to reflect removal of automatic @embeddedkafka annotation
application.yml.ejs Disabled Kafka by default: excluded KafkaAutoConfiguration, disabled function scanning, set test binder as default
application-kafka.yml.ejs New file containing Kafka-specific configuration (function definitions and bindings) loaded when @embeddedkafka is used
files.ts Added conditional generation of application-kafka.yml for Kafka-enabled projects

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

}
} catch (Exception e) {
log.error("Failed to load application-kafka.yml", e);
throw new RuntimeException(e);
Copy link

Copilot AI Jan 17, 2026

Choose a reason for hiding this comment

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

Wrapping the IOException in a RuntimeException loses the specific exception type. Consider using a more specific exception or providing more context in the RuntimeException message to aid debugging. For example, you could use: throw new RuntimeException("Failed to load application-kafka.yml configuration file", e)

Suggested change
throw new RuntimeException(e);
throw new RuntimeException("Failed to load application-kafka.yml configuration file", e);

Copilot uses AI. Check for mistakes.
@mshima
Copy link
Member

mshima commented Jan 17, 2026

Kafka testcontainer doesn't seem to be started at all.
Test doesn’t seem to be functional, wonder if it’s already problematic in main.

@DanielFran
Copy link
Member

Superseded by #32079

@DanielFran DanielFran closed this Jan 27, 2026
@mshima mshima reopened this Jan 27, 2026
@mshima
Copy link
Member

mshima commented Jan 27, 2026

Still relevant.

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.

Kafka container should not be required in some tests.

3 participants