Adjust behat test to only operate on workspace names

mhsdesign commented 2 months ago

I see there are 5 different step syntaxes to change the workspace and or content stream id.

Step	Modifies	Usages
`I am in workspace "live"`	`currentWorkspaceName`	8
`I am in content stream "cs-id"` or `I am in content stream "cs-id" and dimension space point {}`	`currentContentStreamId`	113
`I am in the active content stream of workspace "live"` or `I am in the active content stream of workspace "live" and dimension space point {}`	`currentWorkspaceName` & `currentContentStreamId`	788

We hardly use the logic you introduced - only 8 times - as otherwise the currentContentStreamId will be set.

Also problematic, as you pointed out in slack, is that the step I am in content stream "cs-id" ... will no longer be supported really as api ...

I dont know how what the plan is cc @nezaniel

_Originally posted by @mhsdesign in https://github.com/neos/neos-development-collection/pull/5028#discussion_r1593577914_

nezaniel commented 2 months ago

If we don’t communicate content stream IDs anymore, we don’t need them as testing runtime variables either

mhsdesign commented 1 month ago

Bernhard wrote:

Absolutely, yes. Recommended way forward: 1) merge this 2) remove the remainig usages of test runtime variable currentContentSteamId in PHP, if any 3) rewrite all the test "given"s

And remove the internal testing hack ContentGraphFinder::getByWorkspaceNameAndContentStreamId

mhsdesign commented 1 month ago

i see that ProjectedNodeTrait::iExpectANodeIdentifiedByXToExistInTheContentGraph means that we still write tests like

Then I expect a node identified by cs-identifier;nody-mc-nodeface;{"language":"mul"} to exist in the content graph

but dont we want to have live;nody-mc-nodeface;{"language":"mul"} in the future? Maybe we can have it legacy for cs ids and change this to workspace names if possible?

mhsdesign commented 1 month ago

And remove the internal testing hack ContentGraphFinder::getByWorkspaceNameAndContentStreamId

this might not be easily doable:

Also, we cannot remove content stream checks as we have test cases for content stream forking without workspaces being involved. Also, structure adjustments don't use workspaces, so we have to again check content streams

mhsdesign commented 1 month ago

NodeDiscriminator::fromNode is one of the last usages of $node->subgraphIdentity->contentStreamId do we want to migrate to workspaces here as well or make assertions still on the content stream?

// cc @nezaniel

nezaniel commented 1 month ago

I guess we'll have to resolve the NodeDiscriminators in another way, like using the Node's workspace name, fetching the workspace from the CR and then compare the content stream ID. I'd very much like to keep the tests on content stream level because I don't want to have to even think about possible additional edge cases

bwaidelich commented 1 week ago

as discussed today, we should try to keep the content stream concept as "low-level" as possible. As a result, we agreed to

Keep the CS id out of the public API
Don't introduce new DTOs that link nodes to CS ids (like #5144)

It seems to make sense to still implicitly test, that a node resides in a certain CS and replacing

And I expect a node identified by cs-identifier;nody-mc-nodeface;{"language":"mul"} to exist in the content graph

with something like

And I expect a node identified by live;nody-mc-nodeface;{"language":"mul"} to exist in the content graph

would make this test a little less specific.

As a result, we suggest to keep the current format of describing nodes via their CS (where it makes sense) but to resolve the corresponding workspace name in the respective step implementation.

mhsdesign commented 1 week ago

i forget a point in the weekly. The And I am in content stream "user-cs-identifier" and dimension space point {} (15 usages) draws a line in the sand. It will initialise the currentContentStreamId which will be later used for fetching the currentSubgraph:

https://github.com/neos/neos-development-collection/blob/ea28204af56d75a696fe5b53c8531c65bc297a11/Neos.ContentRepository.TestSuite/Classes/Behavior/Features/Bootstrap/CRTestSuiteRuntimeVariables.php#L150-L153

By using the I am in content stream step here

https://github.com/neos/neos-development-collection/blob/010d976dc7aa8f4c0793bc959653200d1febb795/Neos.ContentRepository.BehavioralTests/Tests/Behavior/Features/ContentStreamForking/ForkContentStreamWithoutDimensions.feature#L57

the workspace of the node will not be correct, in our cases it will actually be live due to the initialiser And I am in workspace "live" above.

We should probably change the step to:

And I am in content stream "user-cs-identifier" aliased by workspace "missing" and dimension space point {}

... that makes this a little more complicated as we cannot use the 'real' workspace finder but have to check first if current content-stream id is set and have to use that, which would not be correct if we have a foreign currentNode:

Then I expect a node identified by migration-cs;sir-david-nodenborough;{"language": "ch"} to exist in the content graph

bwaidelich commented 1 week ago

I would suggest to change that to just

And I am in workspace "<workspace-name>" and dimension space point {}

mhsdesign commented 1 week ago

well there is no workspace at that point... see the mentioned usage or any of the other 15 ones. We would have to create a workspace first and point it there ... but the point of the test was i guess to test the forking only ... and i dont now if CreateWorkspace can be used to glue together an existing content stream id without workspace to a one with workspace ... so we would need to send raw events to get that doneeeeeeeeeee

bwaidelich commented 1 week ago

OK, I see this scenario

    When the command "ForkContentStream" is executed with payload:
      | Key                   | Value                |
      | contentStreamId       | "user-cs-identifier" |
      | sourceContentStreamId | "cs-identifier"      |
    And I am in content stream "user-cs-identifier" and dimension space point {}

won't work – from an API point of view, there is no way to access this node in the forked content stream.. For that case I would suggest to add one step that creates a corresponding workspace – does that work?

For the next scenario:

      # live
    When I am in content stream "cs-identifier" and dimension space point {}
    Then I expect node aggregate identifier "nody-mc-nodeface" to lead to node cs-identifier;nody-mc-nodeface;{}
    And I expect this node to have the following properties:
      | Key  | Value            |
      | text | "original value" |

    # forked content stream
    When I am in content stream "user-cs-identifier" and dimension space point {}
    Then I expect node aggregate identifier "nody-mc-nodeface" to lead to node user-cs-identifier;nody-mc-nodeface;{}
    And I expect this node to have the following properties:
      | Key  | Value            |
      | text | "modified value" |

I would suggest to rewrite

    When I am in workspace "live" and dimension space point {}
    Then I expect node aggregate identifier "nody-mc-nodeface" to lead to node cs-identifier;nody-mc-nodeface;{}
    And I expect this node to have the following properties:
      | Key  | Value            |
      | text | "original value" |

    # forked content stream
    When I am in content stream "user" and dimension space point {}
    Then I expect node aggregate identifier "nody-mc-nodeface" to lead to node user-cs-identifier;nody-mc-nodeface;{}
    And I expect this node to have the following properties:
      | Key  | Value            |
      | text | "modified value" |

mhsdesign commented 1 week ago

won't work – from an API point of view, there is no way to access this node in the forked content stream..

exactly, kinda, thats why we created the internal api (that was the major part of our discussions regarding the content graph adapter change) And one can write - and the test use it (see above):

ContentGraphFinder::getByWorkspaceNameAndContentStreamId(WorkspaceName::fromString('missing'), $anyContentStreamId);

For that case I would suggest to add one step that creates a corresponding workspace – does that work?

Nope. The CreateWorkspace would require a base workspace to exist and thus fork it while the CreateRootWorkspace would just create the content stream id rather than use it if already there which will fail:

https://github.com/neos/neos-development-collection/blob/7db53b6c4ec590973e063077a8c4a2ab72801c5c/Neos.ContentRepository.Core/Classes/Feature/WorkspaceCommandHandler.php#L203-L208

And sending plain WorkspaceWasCreated events via the tests is also not the yellow of the egg... maybe we need a low level CreateWorkspaceForContentStream?

kitsunet commented 1 week ago

I would suggest that those tests are testing low level functinoality "behind the balck box" of the public API, and therefore we might just do the same on the testing side? aka look in the DB or whatever is necessary... A node I cannot observe I cannot test on that level of abstraction. What I can test however if the results in the DB match my expectation.

mhsdesign commented 1 week ago

or we really just introduce the syntax WorkspaceName::fromString('DETATCHED:<ContentStreamId>') ... that way we'd now what to write inside the fetched workspace name of a forbidden node as well :D but i we had the discussion already and at that time decided against it:

9.0 weekly - 01-03-2024

Decide for WorkspaceName|DetachedWorkspaceName $workspaceName later if we really need it.
- Changing this later would be a b/c disaster
- Instead: WorkspaceName::fromString() stricter without colons (so that we can allow for extension, e.g. WorkspaceName::detached(ContentStreamid $csId) (with an internal representation like “cs:”)

edit 1:

-> PR which implements this https://github.com/neos/neos-development-collection/pull/5167

edit 2:

the commit "TASK: Remove deprecated usage $node->subgraphIdentity->contentStreamId" shows how easy it will be to get the possibly detached content stream id from a node, as the content graph finder will do it itself.

mhsdesign commented 6 days ago

I would suggest that those tests are testing low level functinoality "behind the balck box" of the public API, and therefore we might just do the same on the testing side? aka look in the DB or whatever is necessary...

yes that doenst sound so bad as well but on the other hand we have the test now. The following three in Neos.ContentRepository.BehavioralTests/Tests/Behavior/Features/ContentStreamForking depend on content stream assertions:

ForkContentStreamWithDisabledNodesWithoutDimensions.feature
ForkContentStreamWithoutDimensions.feature
NodeReferencesOnForkContentStream.feature

They use simple assertions like I expect node aggregate identifier "nody-mc-nodeface" to lead to node cs-identifier;nody-mc-nodeface;{} and I expect this node to have the following properties but also complex ones like I expect this node to have the following references I expect this node to be referenced by where we heavily use the subgraph reference find query. We definitely profit here from being allowed to do assertions on a real Node and querying the subgraph in this content stream.

Further ForkContentStream seems like a legit command as standalone, so we should probably allow the tests to access the forked content stream as well or even make it user land api? Before our workspace change it was definitely allowed and possible but now the commands seems to be just low level and are not usable despite being declared @api.

If this thesis is true, it means our test are allowed to also use the subgraph in such forked content stream. If not we can probably just omit manually forking and create a workspace instead which will do the forking for us?

kitsunet commented 6 days ago

I guess that's the decision we have to make. Atm it looks like ForkContentStream could very well be internal, but IDK if that is problematic for standalone use cases... @bwaidelich might have to pitch in, but I guess in general that would be a discussion and decision to have on Friday?

mhsdesign commented 6 days ago

Jip, using CreateWorkspace here to implicitly do a ForkContentStream seems the way to go.

Started playing around with this in a third approach 😂 https://github.com/neos/neos-development-collection/pull/5169

see commit WIP: Migrate ContentStreamForking tests to not fork explicitly but use workspaces

This will also allow us to fully get rid of CRTestSuiteRuntimeVariables::$currentContentStreamId AND ContentGraphFinder->getByWorkspaceNameAndContentStreamId which was the goal either way :D Still having this content stream id as runtime variable rather causes bugs, see https://github.com/neos/neos-development-collection/pull/5162

before:

    When the command "ForkContentStream" is executed with payload:
       | Key                   | Value                |
       | contentStreamId       | "user-cs-identifier" |
       | sourceContentStreamId | "cs-identifier"      |
     And I am in content stream "user-cs-identifier" and dimension space point {}

     Then I expect node aggregate identifier "nody-mc-nodeface" to lead to node user-cs-identifier;nody-mc-nodeface;{}

after

    # Uses ForkContentStream implicitly
     When the command CreateWorkspace is executed with payload:
       | Key                | Value                |
       | baseWorkspaceName  | "live"               |
       | workspaceName      | "user-test"          |
       | newContentStreamId | "user-cs-identifier" |

     When I am in workspace "user-test" and dimension space point {}
     Then I expect the workspace to point to content stream "user-cs-identifier"
     Then I expect node aggregate identifier "nody-mc-nodeface" to lead to node user-cs-identifier;nody-mc-nodeface;{}

... we could even leave out the newly added step Then I expect the workspace to point to content stream "user-cs-identifier" as this is asserted via the lead to node thing.

mhsdesign commented 5 days ago

Jip https://github.com/neos/neos-development-collection/pull/5169 is definitely the right direction and in combination with

I expect node aggregate identifier "nody-mc-nodeface" to lead to node user-cs-identifier;... we will still test that the content stream forking went well. Thanks for all the discussion it payed of.

neos / neos-development-collection

Adjust behat test to only operate on workspace names #5034