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

Merge 2.13.x/3.1.x into 3.2.x #4633

Closed
wants to merge 9 commits into from

Conversation

derrabus
Copy link
Member

@derrabus derrabus commented May 2, 2021

Follows #4632

morozov and others added 9 commits April 29, 2021 08:50
[doctrineGH-4613] Use utf8mb4 instead of utf8 for testing connection charset
Display metrics of 3.1 instead of 3.0
* 2.13.x:
  Support Doctrine Cache 2
  Display metrics of 3.1 instead of 3.0
  [doctrineGH-4613] Use utf8mb4 instead of utf8 for testing connection charset
* 3.1.x:
  Support Doctrine Cache 2
  Display metrics of 3.1 instead of 3.0
  [doctrineGH-4613] Use utf8mb4 instead of utf8 for testing connection charset
@morozov
Copy link
Member

morozov commented May 3, 2021

@derrabus what's the difference between 3.1.x and 3.2.x that requires fixing tests? It is possible that there is a BC break.

@derrabus
Copy link
Member Author

derrabus commented May 3, 2021

The DoctrineProvider class transforms the cache key. That's why I'm performing the assertion on the unwrapped adapter.

@morozov
Copy link
Member

morozov commented May 4, 2021

A functional test cannot perform assertions on implementation details – that's why it has to be reworked when implementation details change creating a sense of a BC break. Since caching is by definition a non-functional requirement I'm curious if caching can be tested in a functional way at all.

We already had a similar discussion in #4189 (comment). Should we just get rid of this "functional" test?

@derrabus
Copy link
Member Author

derrabus commented May 6, 2021

Sorry for the late reply. My understanding of the test is "If this operation is performed, make sure that something has been written to the cache". I think, this is a valid thing to test. One could argue if there's a better way to do this, but for this PR (because it's just a merge from a lower branch), my main concern was to make the test work as it had before.

@morozov
Copy link
Member

morozov commented May 6, 2021

If this operation is performed, make sure that something has been written to the cache

This would be the responsibility of a unit test that would have changed as soon as the new implementation was introduced, not during a merge-up.

my main concern was to make the test work as it had before

My main concern is that the caching piece of logic is barely maintainable (not your fault), so the rework of its internals could be used as an opportunity to improve the tests as well. @greg0ire please step in and review.

@derrabus
Copy link
Member Author

derrabus commented May 6, 2021

I've created #4639 to resolve the issue before we merge the branches. Let's continue the discussion there.

@derrabus derrabus closed this May 6, 2021
@derrabus derrabus deleted the merge/3.1.x-in-3.2.x branch May 6, 2021 16:53
@derrabus derrabus mentioned this pull request Jun 5, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants