SLING-12636: Resource Resolver: add test coverage for background init #136
Conversation
| private void initializeVanityPaths() throws IOException { | ||
| getVanityPathsProcessed().set(false); | ||
| mapEntries.initializeVanityPaths(); | ||
| waitForBgInit(); |
There was a problem hiding this comment.
do I get it right, that you added the waitForBgInit() just in preparation for the next change, where you will run tests also with isVanityPathCacheInitInBackground = true?
There was a problem hiding this comment.
The tests (now) run with bginit as well, see parameters of the test class.
|
|
||
| // get vanity paths (after waiting for bg init to complete) | ||
| private void initializeVanityPaths() throws IOException { | ||
| getVanityPathsProcessed().set(false); |
There was a problem hiding this comment.
Why do you need to reset the internal state here?
There was a problem hiding this comment.
to check whether background init has finished; otherwise the expections about the number of entries in the map will be incorrect.
There was a problem hiding this comment.
I still don't follow the full logic; you are replacing doInit with changing an internal flag; this IMO makes the test assume too much about the behaviour of MapEntries and is going to make evolving this class more difficult.
Have you considered adding a new protected/package-private method that will encapsulate the re-reading of vanity paths?
There was a problem hiding this comment.
I'll do that in later PRs.
This change is focused on improving test coverage without refactoring MapEntries. Tests will be updated when refactoring is done.
FWIW, doinit() actually initializes aliases, not vanity paths. All of this will be improved step by step.
There was a problem hiding this comment.
Why do you need to reset the internal state here?
I checked, and it is needed. I currently do not understand what sets the state to "true", but I'll try to find out.
There was a problem hiding this comment.
Ok. The reason is that the internal state of MapEntries did not account for loadVanityPaths called twice.
I added that, and was able to remove getVanityPathsProcessed().set(false); from the test.
Your point about extending MapEntries is understood; however the point of this change was to improve test coverage before doing any changes of what's being tested.
|
… - fix MapEntries internal state when loadVanityPaths is called a second time
|
@rombert , @joerghoh - I plan to go ahead with this once https://issues.apache.org/jira/browse/SLING-12637 (release of 1.12.4) is done. |
No description provided.