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

TASK: 5034 fully completely workspace aware behat tests #5169

Merged

Conversation

mhsdesign
Copy link
Member

@mhsdesign mhsdesign commented Jul 1, 2024

Resolves: #5034
Replaces: #5167
Replaces: #5144

  • Remove CRTestSuiteRuntimeVariables::$currentContentStreamId
  • gets rid of the final usages of I am in content stream in the behat tests.
  • Additionally our hack \Neos\ContentRepository\Core\ContentGraphFinder::getByWorkspaceNameAndContentStreamId is now no longer in use by the tests and can be changed to be not at all exposed.
  • Removes deprecated usage $node->subgraphIdentity->contentStreamId in \Neos\ContentRepository\TestSuite\Behavior\...\NodeDiscriminator::fromNode

Upgrade instructions

Review instructions

Checklist

  • Code follows the PSR-2 coding style
  • Tests have been created, run and adjusted as needed
  • The PR is created against the lowest maintained branch
  • Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
  • Reviewer - The first section explains the change briefly for change-logs
  • Reviewer - Breaking Changes are marked with !!! and have upgrade-instructions

@mhsdesign mhsdesign force-pushed the task/5034-fully-workspace-aware-behat-tests branch from a9e4f86 to 80f01de Compare July 1, 2024 15:42
@kitsunet
Copy link
Member

kitsunet commented Jul 1, 2024

I think I like this direction

mhsdesign added 3 commits July 2, 2024 10:50
… use workspaces

The implicit forking allows us to use workspaces for assertions on the nodes.
… to content stream "user-cs-identifier"`

This was just a temporal idea
@mhsdesign mhsdesign force-pushed the task/5034-fully-workspace-aware-behat-tests branch from 80f01de to e9e4e68 Compare July 2, 2024 09:04
@mhsdesign mhsdesign marked this pull request as ready for review July 2, 2024 09:04
@nezaniel
Copy link
Member

nezaniel commented Jul 5, 2024

Imho If the tests for ForkContentStream are removed, the command itself must be removed as well. Otherwise we have untested code paths and I really dislike that.

@mhsdesign
Copy link
Member Author

We dont remove the tests for ForkContentStream, we just test it from a higher perspective. Effectively the test is the same.
But we could surely just inline the forking logic in the CreateWorkspace command i guess as we wont be needing an api way to just fork a content stream. However if we decide to go down that road i would like to keep that change separate as it will likely also span the CreateContentStream and further commands that are now obsolete standalone.

Copy link
Member

@bwaidelich bwaidelich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me and I like what you wrote.
I agree, that we should mark the low-level commands @internal or even remove them to make forking a purely internal concept

@mhsdesign
Copy link
Member Author

mhsdesign commented Jul 7, 2024

Okay i did declare the content stream commands as internal:

@internal implementation detail, please use the higher level workspace commands instead.

do you agree as well on going down this road now @nezaniel?

edit:

changed to

 * @internal implementation detail. You must not use this command directly.
 * Direct use may lead to hard to revert senseless state in your content repository.
 * Please use the higher level workspace commands instead.

With the content graph / subgraph operating on workspaces, there is no API way of fetching from an arbitrary content stream by id.
And by not being able to access it we declare creating content streams or doing any low level work with it as internal.
@mhsdesign mhsdesign force-pushed the task/5034-fully-workspace-aware-behat-tests branch from 92ab0d3 to 35155bd Compare July 12, 2024 20:47
@mhsdesign mhsdesign merged commit 56e5218 into neos:9.0 Jul 12, 2024
6 of 8 checks passed
@mhsdesign mhsdesign deleted the task/5034-fully-workspace-aware-behat-tests branch July 12, 2024 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adjust behat test to only operate on workspace names
4 participants