-
Notifications
You must be signed in to change notification settings - Fork 74
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 tag latest when fetching dependencies #3879
Conversation
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.
It feels like this could have a test
val saveLatestStates = List(state1, state2, state3, state5).traverse(stateStore.unsafeSave(_)) | ||
val saveTaggedStates = List(state2, state3).traverse(stateStore.unsafeSave(_, UserTag.unsafe("my-tag"))) | ||
(saveLatestStates >> saveTaggedStates).transact(xas.write) |
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 can't really tell what this test is doing, is this affecting the other tests somehow?
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.
It is populating the database with:
- latest states that should be returned in some of the tests
- tagged states that should never been returned in this context
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.
The problem I see is that it's unclear from reading the test how those tagged states would affect anything. It would be better if there was a test that was clearly saying that the tagged versions should not appear (or whatever the assertion should be)
val saveLatestStates = List(state1, state2, state3, state5).traverse(stateStore.unsafeSave(_)) | ||
val saveTaggedStates = List(state2, state3).traverse(stateStore.unsafeSave(_, UserTag.unsafe("my-tag"))) | ||
(saveLatestStates >> saveTaggedStates).transact(xas.write) |
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.
The problem I see is that it's unclear from reading the test how those tagged states would affect anything. It would be better if there was a test that was clearly saying that the tagged versions should not appear (or whatever the assertion should be)
@@ -109,7 +113,7 @@ class EntityDependencyStoreSuite extends BioSuite with Doobie.Fixture { | |||
) | |||
} | |||
|
|||
test("Fetch values for direct dependencies of id1") { | |||
test("Fetch latest state values for direct dependencies of id1") { |
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 doesn't check that the latest state is returned, as both the states appear the same in the result (so you could be returning the tagged version rather than latest)
To communicate the intent of the test better, it would be nice to express "I want the latest version of state 2, and I don't want the other version of state 2"
* Add tag latest when fetching dependencies --------- Co-authored-by: Simon Dumas <[email protected]>
Fixes #3849