apache / karaf-minho

Apache Karaf Minho
https://karaf.apache.org/
Apache License 2.0
6 stars 5 forks source link

Fix the envKey in Config.java #11

Closed iskey closed 1 year ago

iskey commented 1 year ago

fix #10

fpapon commented 1 year ago

@iskey can you add a test to validate please?

jbonofre commented 1 year ago

Thanks, let me take a look.

iskey commented 1 year ago

@iskey can you add a test to validate please?

ok, let me do some tests.

iskey commented 1 year ago

Better to do a mock test. Post two snapshots for contrast for now:

from image

to image

fpapon commented 1 year ago

Better to do a mock test. Post two snapshots for contrast for now:

from image

to image

Sounds good!

rmannibucau commented 1 year ago

If possible try avoiding a mock please, it is like not testing at all at the end. Using surefire you can force some env var so it is then easy to check it is used or not cases IMHO.

jbonofre commented 1 year ago

I think we don't need a mock for the test. We can directly test the ConfigService and setting a system property to test. Actually, I think for this kind of "simple" bug, we can just move forward without test.

jbonofre commented 1 year ago

If no objection, I will merge this PR.

rmannibucau commented 1 year ago

Guess the regex should be extracted if config service is used at runtime instead of recompiled each time.

jbonofre commented 1 year ago

@rmannibucau good point, we can have the Pattern compile as static.

fpapon commented 1 year ago

@jbonofre agree, no mock is needed, go for merge.

iskey commented 1 year ago

Agree, no mock here. Actually, it is hard to mock without extracting the getEnv as another method. Let me make the pattern static.