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

Tests for test result cache depend on PHP implementation details #3581

Closed
sebastianbergmann opened this issue Apr 2, 2019 · 6 comments
Closed
Assignees
Labels
feature/test-runner CLI test runner type/refactoring A refactoring that should be applied to make the code easier to understand and maintain

Comments

@sebastianbergmann
Copy link
Owner

@krakjoe informed me that some of the test failures of #3526 are likely due to changes to serialize() / unserialize(). PHPUnit's tests should not depend on implementation details like that.

@sebastianbergmann sebastianbergmann added the type/refactoring A refactoring that should be applied to make the code easier to understand and maintain label Apr 2, 2019
@epdenouden
Copy link
Contributor

Agreed. I'll upgrade the cache to use a more generic format.

@krakjoe
Copy link
Contributor

krakjoe commented Apr 2, 2019 via email

@epdenouden
Copy link
Contributor

Designing your own format isn't necessary, you can depend on the functionality without depending on exchange format.

Hi @krakjoe! This is very likely true, but the I haven't looked into the specifics of the bug. The ResultCache also uses (un)serialization wrangling of the data. The serialize() format is rather cumbersome to read, making debugging bugs like these a chore without tools. When building the feature I had small workbench set up just for this.

The reason I used the PHP-native serialize() is because I needed a quick way to dump data to a file in a common format that didn't require an external library. I'm guessing the reason this got filed on April 2 is because I would have refactored it with fputcsv() yesterday?

@krakjoe
Copy link
Contributor

krakjoe commented Apr 2, 2019

I've looked a little closer at the failure that provoked me to make the comment that provoked this issue, it may not be related to serial format ...

I'll look closer still when I've got another hour (hopefully later today) ...

Regardless of what is causing this particular failure, I was making the comment that depending on the format of serialized variables is not very smart, because it's not a format we make any guarantees about - if during the release cycle for any version of PHP, we discover that it must change for security (or indeed any other) reasons, then change it must and it will break this kind of test.

If you want to test if serialize/unserialize is working properly, then execute them, but don't depend on the details of the exchange format would be my advice.

@epdenouden
Copy link
Contributor

epdenouden commented Apr 2, 2019

but don't depend on the details of the exchange format would be my advice.

And that's very good advice! :-) I do use the output format in one of the PHPT tests to see what would be written to the filesystem. I'll fix this to better tests the lifecycle of the data in the cache, instead of the cachefile itself.

There are already other tests doing this. Perhaps the end-to-end tests is superfluous already.

@sebastianbergmann sebastianbergmann added the feature/test-runner CLI test runner label Apr 22, 2019
@stale stale bot added the stale label Jul 2, 2019
Repository owner deleted a comment from stale bot Jul 2, 2019
@sebastianbergmann
Copy link
Owner Author

Should be taken care of by 3e3aecd.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/test-runner CLI test runner type/refactoring A refactoring that should be applied to make the code easier to understand and maintain
Projects
None yet
Development

No branches or pull requests

3 participants