-
Notifications
You must be signed in to change notification settings - Fork 8
16 refactor tests #20
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
base: master
Are you sure you want to change the base?
Conversation
…ometry + aggregate relationships for composition
# Conflicts: # pom.xml
…feature/nested-components # Conflicts: # src/main/java/org/reactome/server/graph/domain/model/DatabaseObject.java
…-refactor-tests # Conflicts: # src/main/java/org/reactome/server/graph/aop/README.md # src/test/java/org/reactome/server/graph/service/AdvancedServiceTest.java # src/test/java/org/reactome/server/graph/service/DiagramServiceTest.java # src/test/java/org/reactome/server/graph/service/ParticipantServiceTest.java
EliotRagueneau
left a comment
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.
Quite a few bits to change:
- Use static variable as much as possible instead of raw string, specially in code base but also in tests
- Avoid breaking logic of tests when refactoring. If you do not understand the difference between 2 tests, or that the logic is completely confusing to you, please ask instead of assuming the tests are just wrong. It might be the case, but it is worth asking in case the logic might actually be relevant, because in most cases it actually is
- Make sure to use the ofrmatter from intellij when coding, cmd + opt + L should be a reflex by now
Overall big work and efforts, greatly appreciated, but I think you should have been a bit more patient with some tests and really have tried to understand what was intended to be tested, because you miss the point of quite a few tests, and even deleted some. the aim is of course not to always have all tests green, otherwise you would just have no test at all and problem solved ;)
| <img src=https://cloud.githubusercontent.com/assets/6883670/22938783/bbef4474-f2d4-11e6-92a5-07c1a6964491.png width=220 height=100 /> | ||
|
|
||
| Aspect-Oriented Programming with Spring | ||
| Aspect Oriented Programming with Spring |
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.
that was better before
| :warning: Changes in this package have to be taken carefully, considering it is going to reflect in the whole application | ||
|
|
||
| ### What is Aspect-Oriented Programming - AOP? | ||
|
|
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.
that was better before
| ### What is Aspect-Oriented Programming - AOP? | ||
|
|
||
| Spring AOP framework is used to modularize cross-cutting concerns in aspects. Simply, it’s just an interceptor to intercept some processes, for example, when a method is executed, Spring AOP can hijack the executing method, and add extra functionality before or after the method execution. The most used feature is logging. | ||
| Spring AOP framework is used to modularize cross-cutting concerns in aspects. Simply, t’s just an interceptor to intercept some processes, for example, when a method is execute, Spring AOP can hijack the executing method, and add extra functionality before or after the method execution. The most used feature is logging. |
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.
that was better before
| } | ||
|
|
||
| public void setDeletedList(List<Deleted> deleted) { | ||
| public void setDeleted(List<Deleted> deleted) { |
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.
Unsure about this one, need to be tested to be sure it's working
| import org.springframework.data.neo4j.core.schema.Node; | ||
| import org.springframework.data.neo4j.core.schema.Relationship; | ||
|
|
||
| import javax.management.relation.Relation; |
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.
Wrong import?
| package org.reactome.server.graph.util; | ||
|
|
||
| public enum ReactionType { | ||
| ASSOCIATION,DISSOCIATION,TRANSITION, BINDING |
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.
formatting pls
| @Repository | ||
| public class TestNodeRepository { | ||
|
|
||
| private final Neo4jClient neo4jClient; |
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.
Format
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.
Please add //language=Cypher before queries
| @Test | ||
| public void testGetSpeciesAnyId(){ | ||
| Species species = speciesService.getSpecies(9606); | ||
| Species species = speciesService.getSpecies(testSpecies.getDbId()); |
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.
taxId
| @SuppressWarnings("Duplicates") | ||
| @Test | ||
| public void libraryStabilityTest() { | ||
| public void libraryStabilityTest() { // This test makes no sense |
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.
I agree, I think there is made because sometimes, when we queried twice for the same thing with another query in the middle, then the second one wasn't the same. This is ensuring this is not the case with the current library version we are using of SDN. Please keep something similar
| } | ||
|
|
||
| @Test | ||
| public void lazyLoadingStoichiometryTest(){ |
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.
This is to test that you get the same result by lazy loading a property, and by directly querying for it. It also checks that the stoichiometry expansion works in both cases. Please reintegrate similar test
PR for test refactoring