How to segregate imports of module being 'aware of' away from Spring wiring?

Development
,
21

Hi everyone.

I got it working thanks to @dkayiwa’s support! There’s a caveat regarding unit tests tho. I’ll put everything together and report back about the inner details on Monday or so. And I’ll be able to finally do this PR for ADDR-108.

22

Great news! Keep me posted…

23

So… It works and here are a few things to know as per my latest understanding of things:

  1. Segregate and isolate all imports from the module being aware of into one bean implementation class.
  2. That bean is entirely defined for Spring through its @OpenmrsProfile annotation. There should be (1) no entry in the application context XML file and (2) no @Component annotation. Those would trigger Spring to wire that bean ahead of the OpenMRS-specific conditions leading to runtime trouble. All this implies that the bean won’t have a known bean ID, meaning that…
  3. This bean can only be @Autowired, and even presumably @Autowired(required = false) depending on the conditions.

All the above can lead to a slight headache on the context-sensitive tests side of things. Hence those two new topics on which I would be eager to get input:

@mogoodrich basically this all means that the PR for ADDR-108 is being delayed… :unamused:

24

Thanks @mksd! If there’s a short-term fix to work around test issues while we figure out the “right” way to go about it, feel free to submit.

Take care, Mark

25

Also @mksd and @mogoodrich - remember, any addition to what the core OpenmrsProfile annotation can do, or what base unit test cases are available will lead to us needing to increase the minimum supported OpenMRS version in the 1.9 and beyond release lines. This may not be desirable and should be evaluated.

Mike

26

@mksd i do not seem to understand what the blocker is. I thought all tests are now passing and the required runtime functionality works as expected.

27

The tests are not passing, no. Because by setting the entry in TestingApplicationContext.xml as you suggested, we would autowire the same implementation of I18nCache for all tests. But certain tests cover the case when Ext I18N is present, and other tests cover the cases when Ext I18N is missing.

Knowing this I am assessing whether I could push for changes that would allow the runtime configuration to be handled seamlessly within context-sensitive tests as well.

Unfortunately as @mseaton just hinted, this might be too costly for their team. AH would need to require Core 1.9.12.

I also understand that there are workarounds, but I am keen on getting this done as neatly as possible because we want to advocate for a similar (yet less intrusive) effort to be made within Bahmni Core as well. This is why I’m wishing for an adequate handling of the modules startup within unit tests, and for a wider scenario coverage for @OpenmrsProfile.

28

Are your changes committed for us to take a look at the failing tests?

29

Not to sidetrack us too much, but, for what it’s worth, I don’t know much of the details of the exti18n module, but if we think it’s stable and going to be a core part of our internationalization strategy going forward, we could consider just making address hierarchy fully depend on exti18n (and add it to the Ref App and the PIH EMR distro).

30

@dkayiwa, not really just right now as I have been testing multiple things locally. Thanks for offering to look tho. I know that there are workarounds, and that I can achieve to have a working version using a one or another.

But I’m testing the waters here:

  • Would it be possible to enrich the test framework? (this thread).
  • Is it sensible to extend @OpenmrsProfile's behaviour? (that other thread).
  • If both the above can be considered, could I use both for our real world case: AH aware of Ext I18N?
  • Is that acceptable for the maintainers of AH?
  • Could that pave the way for a very similar approach in Bahmni Core?

@mogoodrich, I’m glad to read that you guys would consider adopting Ext I18N as you said. However let’s keep in mind that Ext I18N is a workaround in itself. I would wish for higher versions of the Core (3.0?) that would not necessitate it anymore, In that light it makes sense to keep it as an optional artifact, with everything running smoothly when it’s not there.

31

@mksd regardless of what the AH maintainers decide to go for, it will still be awesome to enrich the test framework and also extend the @OpenmrsProfile’s behavior. :slight_smile:

1 Like
32

Thanks @dkayiwa, that’s supportive!

33

After months of running this on our own CI, we are now in a position to PR a (yet much smaller) change, but similar in nature, to Bahmni Core…
…since we finally have a development environment with OpenMRS Core 2.1.1 (bringing @OpenmrsProfile loading beans based on modules not started)!

Bahmni Core API would depend on Ext I18N 1.0.0. However Bahmni Core would only be aware of Ext I18N and only one single bean’s implementation does in fact refer to Ext I18N, the one bean that is loaded if the module is present (see here on our fork).

This technique has worked well with AH and is part of its release 2.11.0. And as I said it has worked well with Bahmni Core as well since we have been running a full English-Khmer version of Bahmni on our servers for months.

I need to squash my commits and bring all this to master and I shall have a PR ready by tomorrow. Giving here the heads up for someone to be on the fence for reviewing it.

Cc @angshuonline @arjun @shruthipitta