-
Notifications
You must be signed in to change notification settings - Fork 17
Only prime cache, if it is available #1538
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
77 changes: 77 additions & 0 deletions
77
src/test/java/org/commcare/cases/entity/AsyncNodeEntityFactoryTest.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| package org.commcare.cases.entity; | ||
|
|
||
| import org.commcare.suite.model.Detail; | ||
| import org.commcare.suite.model.DetailField; | ||
| import org.commcare.suite.model.Text; | ||
| import org.javarosa.core.model.condition.EvaluationContext; | ||
| import org.javarosa.core.model.instance.TreeReference; | ||
| import org.javarosa.core.util.OrderedHashtable; | ||
| import org.javarosa.xpath.expr.XPathExpression; | ||
| import org.junit.Test; | ||
|
|
||
| import java.util.ArrayList; | ||
| import java.util.Hashtable; | ||
| import java.util.List; | ||
| import java.util.Vector; | ||
|
|
||
| import static org.junit.Assert.assertEquals; | ||
|
|
||
| /** | ||
| * Regression test for USH-6551: sort field expressions must not be evaluated during the cache | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Curious if it's a common practice on USS to add Jira ticket # in code comments, we have refrained from doing so on mobile team in past as the code is open source and putting these refrences make it cryptic for anyone external looking into the code. |
||
| * priming loop when no EntityStorageCache is present (formplayer always passes null). | ||
| * | ||
| * Before the fix, setUnCachedDataOld iterated every entity × every column and called | ||
| * getSortField unconditionally, evaluating expensive XPath even though there was nowhere to | ||
| * store the result. | ||
| */ | ||
| public class AsyncNodeEntityFactoryTest { | ||
|
|
||
| @Test | ||
| public void prepareEntities_doesNotCallGetSortField_whenCacheIsNull() { | ||
| Detail detail = buildDetailWithSortField(); | ||
|
|
||
| AsyncNodeEntityFactory factory = new AsyncNodeEntityFactory( | ||
| detail, new EvaluationContext(null), null, false); | ||
|
|
||
| SpyAsyncEntity spy1 = new SpyAsyncEntity(detail); | ||
| SpyAsyncEntity spy2 = new SpyAsyncEntity(detail); | ||
| List<Entity<TreeReference>> entities = new ArrayList<>(); | ||
| entities.add(spy1); | ||
| entities.add(spy2); | ||
|
|
||
| factory.prepareEntities(entities); | ||
|
|
||
| assertEquals("getSortField must not be called when cache is null", 0, spy1.getSortFieldCallCount); | ||
| assertEquals("getSortField must not be called when cache is null", 0, spy2.getSortFieldCallCount); | ||
| } | ||
|
|
||
| private static Detail buildDetailWithSortField() { | ||
| DetailField.Builder fieldBuilder = new DetailField.Builder(); | ||
| fieldBuilder.setSortOrder(1); | ||
| fieldBuilder.setSort(Text.PlainText("sort_value")); | ||
|
|
||
| Vector<DetailField> fields = new Vector<>(); | ||
| fields.add(fieldBuilder.build()); | ||
|
|
||
| return new Detail("test_detail", null, null, null, | ||
| new Vector<>(), fields, new OrderedHashtable<>(), | ||
| null, null, null, null, null, null, null, null, null, null, | ||
| false, false, null); | ||
| } | ||
|
|
||
| private static class SpyAsyncEntity extends AsyncEntity { | ||
| int getSortFieldCallCount = 0; | ||
|
|
||
| SpyAsyncEntity(Detail detail) { | ||
| super(detail, new EvaluationContext(null), new TreeReference(), | ||
| new Hashtable<String, XPathExpression>(), null, null, null); | ||
| } | ||
|
|
||
| @Override | ||
| public String getSortField(int i) { | ||
| getSortFieldCallCount++; | ||
| return super.getSortField(i); | ||
| } | ||
| } | ||
|
|
||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if we would want put this behind a feature flag for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like
commcare-coredoesn't have access to feature flags, is that right? We could pass through a boolean if we needed to - might be nice to easily toggle on and off for performance testing, but coordinating across three repos is tedious, and Shubham seemed optimistic about this change, so maybe not worth the effort. Dunno, I could go either way.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I am quite confident for this to be rolloed out without a feature flag.