ACS-5471: Secondary path support #2585
Conversation
- fixed description
# Conflicts: # pom.xml
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
PMD found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.
- fixes for PMD
- fixes for PMD
.../src/test/java/org/alfresco/elasticsearch/reindexing/NodesSecondaryChildrenRelatedTests.java
Fixed
Show fixed
Hide fixed
- fixes for PMD
- fixes for PMD
krdabrowski
requested review from
tpage-alfresco,
mpichura,
Pawel-608,
MateuszHY and
azakrzewski-hy
| * += fS | ||
| * </pre> | ||
| * Parent += Child - primary parent-child relationship | ||
| * Parent +- Child - secondary parent-child relationship |
There was a problem hiding this comment.
This is really helpful - thanks!
...src/test/java/org/alfresco/elasticsearch/reindexing/NodesSecondaryAncestorIndexingTests.java
Outdated
Show resolved
Hide resolved
...src/test/java/org/alfresco/elasticsearch/reindexing/NodesSecondaryAncestorIndexingTests.java
Outdated
Show resolved
Hide resolved
...src/test/java/org/alfresco/elasticsearch/reindexing/NodesSecondaryAncestorIndexingTests.java
Outdated
Show resolved
Hide resolved
...src/test/java/org/alfresco/elasticsearch/reindexing/NodesSecondaryAncestorIndexingTests.java
Outdated
Show resolved
Hide resolved
|
|
||
| // when | ||
| STEP("Delete the secondary parent-child relationship between folderQ and FolderR."); | ||
| folders(Q).removeSecondaryChild(folders(R)); |
There was a problem hiding this comment.
I think there's a risk that this test will be intermittent. I think we've seen the worst ES offenders are tests which change paths and then expect updates to happen soon afterwards. Could we instead change this to perform the add and remove in the precondition?
Alternatively we could just keep an eye out to see whether this test actually does fail frequently or not. Perhaps it will be fine.
There was a problem hiding this comment.
I understand your point, but before PTO I'm not able to fix all the tests, which change paths and then expect updates to happen soon afterwards. Lets keep eye on them and I can fix them after my PTO.
| initializers = AlfrescoStackInitializer.class) | ||
| public abstract class NodesSecondaryChildrenRelatedTests extends AbstractTestNGSpringContextTests | ||
| { | ||
| protected static final String A = "A", B = "B", C = "C", K = "K", L = "L", M = "M", P = "P", Q = "Q", R = "R", S = "S", X = "X", Y = "Y", Z = "Z"; |
There was a problem hiding this comment.
I think this is a reasonable use of multiple declarations on a single line even if PMD doesn't. I suggest marking it with @SuppressWarnings("PMD.whatever").
There was a problem hiding this comment.
Good hint - the annotation - thanks!
| Map<String, Folder> createNestedFolders(String... folderSuffixes); | ||
|
|
||
| void delete(Folder folder); | ||
| } |
There was a problem hiding this comment.
I think most of the helpers in this class would be widely useful. Perhaps we could consider adding them to the TAS framework in com-repo instead? I don't think this needs to be a base class for tests does it?
There was a problem hiding this comment.
I agree, it's already crossed my mind too, but I also think that firstly this logic should be adapted for more general use - this could be done in a separate task
…ch/reindexing/NodesSecondaryAncestorIndexingTests.java Co-authored-by: Tom Page <[email protected]>
…ch/reindexing/NodesSecondaryAncestorIndexingTests.java Co-authored-by: Tom Page <[email protected]>
…ch/reindexing/NodesSecondaryAncestorIndexingTests.java Co-authored-by: Tom Page <[email protected]>
…ch/reindexing/NodesSecondaryAncestorIndexingTests.java Co-authored-by: Tom Page <[email protected]>
# Conflicts: # pom.xml
# Conflicts: # pom.xml
More info: https://alfresco.atlassian.net/browse/ACS-5471