Skip to content

Conversation

gsmet
Copy link
Member

@gsmet gsmet commented Mar 2, 2019

Fixes #270.

@gsmet
Copy link
Member Author

gsmet commented Mar 3, 2019

@dmlloyd could you take a look at this one?

There's something weird with the config injected in the templates when running several tests.

First, get the code of this PR, then:

cd extensions/agroal/
mvn clean install -Dsurefire.runOrder=alphabetical

-> the tests pass

Now let's run them in reverse order:

mvn clean install -Dsurefire.runOrder=reversealphabetical

-> the second test fails with a NPE

AFAICS, if NoConfigTest is executed first, when DefaultDataSourceConfigTest is executed:

  • the runtime and build time config are correct in AgroalProcessor: the values defined in the config file are there;
  • the runtime and build time config injected in AgroalTemplate are incorrect: it looks as if it gets the empty config of the first test

Could it be that somehow the config values of the first test are cached somehow (maybe a static field in a shared class loader?) injected in the template?

@dmlloyd
Copy link
Member

dmlloyd commented Mar 3, 2019

Yes the present code loads config in a static initialization block and will not reload it unless the class is dropped and reloaded. That might be what you're seeing; I'm not sure what the current unit tests are doing.

@gsmet
Copy link
Member Author

gsmet commented Mar 3, 2019

@dmlloyd are you talking specifically about the templates?

Because my 2 tests are QuarkusUnitTest so they generate a jar and start a new Quarkus app each time.

As mentioned, the config is properly injected in the Processor: the values are correct there.

It's just the values injected in the templates that are incorrect and seems to depend on the order of the tests. I pass the config as parameters of the template methods.

@dmlloyd
Copy link
Member

dmlloyd commented Mar 3, 2019

It looks as if the config is never loaded. Still looking into why.

BTW the processors load the build config, which doesn't come into the equation in terms of causing or solving this problem.

@gsmet
Copy link
Member Author

gsmet commented Mar 3, 2019

Well somehow it can load the config as if you reverse the order of the tests, everything works like a charm.

@dmlloyd
Copy link
Member

dmlloyd commented Mar 3, 2019

I think it might be a class init order problem. If the config helper gets initialized first, then the config is correctly loaded and parsed. If not, then something gets broken with config parsing.

In my work branch I've made the config loading be independent of class initialization. So I think I'll see if I can get that working without the class loader rework, and then see if your test passes then.

@gsmet
Copy link
Member Author

gsmet commented Mar 3, 2019

@dmlloyd if you think it's not practical to fix that now, I can force the order of the tests in the Surefire config.

I just think that's an issue that early adopters could encounter so if we can get to the bottom of it before the launch, that would be nice.

@dmlloyd
Copy link
Member

dmlloyd commented Mar 3, 2019

Things are a mess. We have extensions mixing static init config, build, and run time in invalid ways. I'm working through cleaning it up; I think for now it's best to hard code the tests but I will try and get this at least somewhat solved for the first release.

@gsmet
Copy link
Member Author

gsmet commented Mar 3, 2019

@dmlloyd OK, I will hardcode the orde of the tests.

BTW, is there anything wrong with how I access the config in the Agroal extension? I thought I was doing it right as you mentioned passing the runtime config to a template method several times.

Maybe we should try to write something about how things should be done. I can do it if you tell me what we should do and what we should not.

@dmlloyd
Copy link
Member

dmlloyd commented Mar 3, 2019

It seems correct. I think there's something else wrong though. I'm just having a hard time pinning it down.

@gsmet
Copy link
Member Author

gsmet commented Mar 5, 2019

@Sanne I think we need this one for first public release as having a NPE when adding a dependency is really not cool.

I worked around the config issue for now by strictly ordering my tests (it's an issue only for the tests AFAICS).

@Sanne Sanne merged commit fdf8a20 into quarkusio:master Mar 5, 2019
@Sanne Sanne deleted the agroal-noconfig branch March 5, 2019 13:39
@Sanne Sanne added this to the 0.11.0 milestone Mar 5, 2019
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.

4 participants