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

Remove calls to TestCase::at() #345

Merged
merged 1 commit into from
Aug 10, 2020

Conversation

greg0ire
Copy link
Member

@greg0ire greg0ire commented Aug 1, 2020

It has been deprecated as of PHPUnit 9
See sebastianbergmann/phpunit#4297

Note that we still rely on the call order here, I am not the original
author of this test but the order looks like it is pretty important for
this test, or maybe at least for understand it.
Also note that we no longer check that save() is called after all calls
to fetch().

@greg0ire greg0ire force-pushed the refactor-away-from-at branch 2 times, most recently from 4e06f2f to f99bbe1 Compare August 1, 2020 13:40
@greg0ire
Copy link
Member Author

greg0ire commented Aug 1, 2020

the order looks like it is pretty important for this test, or maybe at least for understand it.

I noticed transient tests that have to do with fetch before making this PR, and I just noticed several ones, so maybe we should in fact get completely rid of the order? 🤔 Not sure how to do that though…

@greg0ire greg0ire force-pushed the refactor-away-from-at branch from f99bbe1 to 2e7939b Compare August 1, 2020 13:54
@greg0ire
Copy link
Member Author

greg0ire commented Aug 1, 2020

Cc @bastnic in case you have ideas

Copy link
Member

@SenseException SenseException left a comment

Choose a reason for hiding this comment

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

I'm not sure if the order of the fetch calls are important for the tests. What if we use a functional test about the fetch -> save relation?

[$this->equalTo($cacheKeyMethod2)],
[$this->equalTo('[C]' . $cacheKeyMethod2)]
)
->will($this->onConsecutiveCalls(
Copy link
Member

Choose a reason for hiding this comment

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

willReturnOnConsecutiveCalls() should do the trick.

@greg0ire
Copy link
Member Author

greg0ire commented Aug 2, 2020

What if we use a functional test about the fetch -> save relation?

Maybe that would help. What would that look like?

@SenseException
Copy link
Member

Maybe that would help. What would that look like?

It could be a test that is using a real cache implementation like a custom one or maybe the ArrayCache and then check afterwards the expected content in the Cache class. I haven't checked if there are already tests like that. A custom test double implementing Cache could contain the checks for order, but I'm more in favour for already existing classes.

@greg0ire greg0ire force-pushed the refactor-away-from-at branch 2 times, most recently from 4c76007 to fdb4ca4 Compare August 7, 2020 20:00
@greg0ire greg0ire marked this pull request as ready for review August 7, 2020 20:04
@greg0ire
Copy link
Member Author

greg0ire commented Aug 7, 2020

Well… I restarted the build several times tonight and could not reproduce the issue, so I guess it was more transient than I thought, meaning it's probably not related to my changes. @SenseException , I made the changes you requested, please review again.

It has been deprecated as of PHPUnit 9

Note that we still rely on the call order here, I am not the original
author of this test but the order looks like it is pretty important for
this test, or maybe at least for understanding it.
Also note that we no longer check that save() is called after all calls
to fetch().
@greg0ire greg0ire force-pushed the refactor-away-from-at branch from fdb4ca4 to dd251d7 Compare August 8, 2020 21:24
@greg0ire greg0ire merged commit 8148dfe into doctrine:1.10.x Aug 10, 2020
@greg0ire greg0ire deleted the refactor-away-from-at branch August 10, 2020 17:24
@greg0ire greg0ire added this to the 1.10.4 milestone Aug 11, 2020
jkufner added a commit to smalldb/annotations that referenced this pull request Aug 11, 2020
Release [1.10.4](https://github.com/doctrine/annotations/milestone/25)

1.10.4
======

- Total issues resolved: **0**
- Total pull requests resolved: **8**
- Total contributors: **5**

 - [347: Add support for the new PHP 8 tokens for use statements](doctrine#347) thanks to @stof
 - [345: Remove calls to TestCase::at()](doctrine#345) thanks to @greg0ire
 - [343: Allow using PHPUnit 9.3](doctrine#343) thanks to @greg0ire

Improvement
-----------

 - [342: Upgrade phpunit](doctrine#342) thanks to @greg0ire
 - [332: DocParser: Improve private typehints](doctrine#332) thanks to @jkufner

bug
---

 - [341: Make type in phpdoc resolvable](doctrine#341) thanks to @greg0ire

Documentation
-------------

 - [338: update annotation IDE annotation with current links](doctrine#338) thanks to @Haehnchen

Documentation,Improvement
-------------------------

 - [337: Replace "blacklist" terminology with "ignore"](doctrine#337) thanks to @albe
Yurunsoft added a commit to Yurunsoft/doctrine-annotations that referenced this pull request Nov 29, 2020
Release [1.10.4](https://github.com/doctrine/annotations/milestone/25)

1.10.4
======

- Total issues resolved: **0**
- Total pull requests resolved: **8**
- Total contributors: **5**

 - [347: Add support for the new PHP 8 tokens for use statements](doctrine#347) thanks to @stof
 - [345: Remove calls to TestCase::at()](doctrine#345) thanks to @greg0ire
 - [343: Allow using PHPUnit 9.3](doctrine#343) thanks to @greg0ire

Improvement
-----------

 - [342: Upgrade phpunit](doctrine#342) thanks to @greg0ire
 - [332: DocParser: Improve private typehints](doctrine#332) thanks to @jkufner

bug
---

 - [341: Make type in phpdoc resolvable](doctrine#341) thanks to @greg0ire

Documentation
-------------

 - [338: update annotation IDE annotation with current links](doctrine#338) thanks to @Haehnchen

Documentation,Improvement
-------------------------

 - [337: Replace "blacklist" terminology with "ignore"](doctrine#337) thanks to @albe

# gpg: Signature made Tue Aug 11 20:55:32 2020
# gpg:                using DSA key 1BEDEE0A820BC30D858F9F0C2C3A645671828132
# gpg: Can't check signature: No public key

# Conflicts:
#	composer.json
#	lib/Doctrine/Common/Annotations/Annotation/Attributes.php
#	lib/Doctrine/Common/Annotations/DocParser.php
#	tests/Doctrine/Tests/Common/Annotations/AbstractReaderTest.php
#	tests/Doctrine/Tests/Common/Annotations/AnnotationReaderTest.php
#	tests/Doctrine/Tests/Common/Annotations/DocParserTest.php
#	tests/Doctrine/Tests/Common/Annotations/FileCacheReaderTest.php
#	tests/Doctrine/Tests/Common/Annotations/SimpleAnnotationReaderTest.php
#	tests/Doctrine/Tests/Common/Annotations/Ticket/DCOM55Test.php
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants