-
Notifications
You must be signed in to change notification settings - Fork 16
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
Add findByCurrentStatus method with sort parameter to order by contributionDeadline #468
Add findByCurrentStatus method with sort parameter to order by contributionDeadline #468
Conversation
@@ -40,6 +41,9 @@ | |||
@Query("{ 'currentStatus': {$in: ?0} }") | |||
List<Task> findByCurrentStatus(List<TaskStatus> statuses); | |||
|
|||
@Query("{ 'currentStatus'; {$in: ?0} }") |
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.
@Query("{ 'currentStatus'; {$in: ?0} }") | |
@Query("{ 'currentStatus': {$in: ?0} }") |
8360653
to
7bec98e
Compare
* the main thread, since deals can have | ||
* a large number of tasks (BoT). | ||
*/ | ||
@Async | ||
@EventListener(ApplicationReadyEvent.class) |
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.
Nice!
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.
Well done 👏
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.
Great, thank you for your tests!
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.
Thanks!
tasks.add(task); | ||
} | ||
taskRepository.saveAll(tasks); | ||
tasks.sort(Comparator.comparing(Task::getCurrentStatus, Comparator.reverseOrder()) |
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 think it is interesting/clearer to add at least 1 test where expected task list is explicitly defined.
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.
Would it be alright to have a simple test with fixed data and a fuzzy one with randomization to try different combinations ?
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 would be perfect, having both benefits!
import static com.iexec.core.task.TaskStatus.RUNNING; | ||
import static com.iexec.core.task.TaskTestsUtils.getStubTask; | ||
|
||
@DataMongoTest |
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.
Are we sure we want Spring tests in our unit tests?
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.
Those are specific tests which only create beans for entities and repositories. The beans annotated with @components and @Services are not created.
The new tests specifically target and validate the data model which should be validated as early as possible.
There is a valid discussion on the topic and we decided to start this way and maybe to refactor later.
Draft PR for comments
To be complete, this feature would merit a basic integration test to check the mongodb query is correct and returns tasks in the expected order.