Closed farost closed 5 months ago
Do not edit the content of this comment. The PR reviewer should simply update this comment by ticking each review item below, as they get completed.
Trivial Change
Code
Architecture
I added some new TODOs for me to address. I'll add a new comment here when the PR is ready for a new review cycle. Otherwise, you can check my answers to the previous comments and check the new commits!
Had a huge update over all the tests in this PR based on the implemented logic on the server side!
There are too many details to mention, all the valuable discussions and decisions were highlighted in our Discord.
There is still a number of TODOs for future work on schema validation, not implemented features and small stuff, but it looks like we can merge it in this form and address it later.
Currently, all the tests in type
pass for my branch with validations and fixes: https://github.com/vaticle/typedb/pull/7094
Honestly, I feel like there are some of the tests written for some of edges (owns/relates/plays) that are not explicitly covered by other edges, but there is too many things in the scope at the moment. I tried to cover everything thoroughly and can guarantee 95% of the cases to be covered by the tests, the rest I'm sure we'll check through test coverage checks later
Usage and product changes
We refactor already existing BDDs for
concept/type
package, adding new base cases of type manipulations and refactoring already existing cases for better scalability in the future.We add tests for all the annotations from
3.0
and basic lists (of roles and of owns) manipulations for:The ultimate goal of this PR is to have decent coverage for schema and refactor the
concept/type
tests so this format could be reused inconcept/thing
later on, additionally being available for small additions on the schema level.Moreover, all the files now tend to have structure for easier test cases search:
Implementation
I won't put many additional comments inside the steps declarations, here is the whole process of additions and refactoring and my logic behind it:
Setup:
owns.feature
andplays.feature
.entitytype
andrelationtype
tests that checkowns
behavior toowns.feature
and tests that checkplays
behavior toplays.feature
.relates
behavior (roles
on the concept level) remains a part of theattributetype
file.Owns:
entitytype
andrelationtype
files tests, putting most of them under aScenario Outline
withExamples
of entities and relations (I add these entities and relations to theBackground
step so they always exist for Examples substitutions). Some of them are left separated so as not to spamentitytype
/relationtype
Xattribute-valuetype
permutations. I also added some more declaration basic and inheritance cases.owns smth[]
as it's easier to add special cases for lists later, andExamples
forScenario Outlines
would cause too much trouble for this case.annotation
s: base cases of declaration (with fails), inheritance, redeclarations, etc.Plays: I copied the base of
owns
tests and followed the same ideas. The main differences here:me sub relation, relates smth[]; ... plays me:smth
), no tests for lists ofplays
as we don't support it yet.@card
, so there are much fewer cases for annotationsEntity type and Relation type: Followed a similar approach as for
plays
, haven't refactored old tests much, but tried to cover annotations decently. Relation types also received additional special tests forrelates
(roles
) and lists ofrelates
.Attribute type: Closer to
owns
for easier changes and scalability regardingannotation
s, but active in using the old tests for simple cases.Addons:
type(name) get smth(name) get smthelse contains:
, etc. Now for the schema level, but I'll be able to propagate this refactoring everywhere once we approve it.TODO
s that require our revisiting later / while fixing the code based on these tests / right now as a part of the review. I'll highlight the main ones by my comments so we can address them here. All the otherTODO
s are much easier to be addressed while working with code.