Skip to content
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

Adjust behat test to only operate on workspace names #5034

Closed
mhsdesign opened this issue May 8, 2024 · 18 comments · Fixed by #5169
Closed

Adjust behat test to only operate on workspace names #5034

mhsdesign opened this issue May 8, 2024 · 18 comments · Fixed by #5169
Assignees
Labels

Comments

@mhsdesign
Copy link
Member

mhsdesign commented May 8, 2024

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 #5028 (comment)

see also https://neos-project.slack.com/archives/C04PYL8H3/p1714846896689819 and https://neos-project.slack.com/archives/C04PYL8H3/p1714742146900109

@nezaniel
Copy link
Member

nezaniel commented May 8, 2024

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

@mhsdesign
Copy link
Member Author

mhsdesign commented May 9, 2024

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
Copy link
Member Author

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
Copy link
Member Author

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 mhsdesign changed the title Adjust behat test to only select on workspace names Adjust behat test to only operate on workspace names May 21, 2024
@mhsdesign
Copy link
Member Author

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
Copy link
Member

nezaniel commented May 21, 2024

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
Copy link
Member

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

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
Copy link
Member Author

mhsdesign commented Jun 28, 2024

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:

if (isset($this->currentContentStreamId)) {
// This must still be supported for low level tests, e.g. for content stream forking
return $contentGraphFinder->getByWorkspaceNameAndContentStreamId($this->currentWorkspaceName, $this->currentContentStreamId)->getSubgraph($this->currentDimensionSpacePoint, $this->currentVisibilityConstraints);
}

By using the I am in content stream step here

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
Copy link
Member

bwaidelich commented Jun 28, 2024

I would suggest to change that to just

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

@mhsdesign
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

mhsdesign commented Jun 28, 2024

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:

$newContentStreamId = $command->newContentStreamId;
$commandHandlingDependencies->handle(
CreateContentStream::create(
$newContentStreamId,
)
);

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

@kitsunet
Copy link
Member

kitsunet commented Jun 28, 2024

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
Copy link
Member Author

mhsdesign commented Jun 28, 2024

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 #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
Copy link
Member Author

mhsdesign commented Jul 1, 2024

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
Copy link
Member

kitsunet commented Jul 1, 2024

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
Copy link
Member Author

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

Started playing around with this in a third approach 😂 #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 #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
Copy link
Member Author

Jip #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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants