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

Deprecate assertArraySubset() #3494

Closed
sebastianbergmann opened this issue Jan 22, 2019 · 25 comments
Closed

Deprecate assertArraySubset() #3494

sebastianbergmann opened this issue Jan 22, 2019 · 25 comments

Comments

@sebastianbergmann
Copy link
Owner

@sebastianbergmann sebastianbergmann commented Jan 22, 2019

The assertArraySubset() method is a constant source of confusion and frustration. For example, see #2069, #2108, #2568, #3101, #3234, #3240, or #3319.

I have come to the conclusion that this mess cannot be fixed without breaking backward compatibility. In addition to that, I myself have yet to see a use case for this or how this would be used then. I have also not seen this used in the wild.

This is why I decided to deprecate assertArraySubset() in PHPUnit 8 and to remove it in PHPUnit 9. Anybody who thinks this functionality is useful is more than welcome to take the code, put it into a separate project, and package it as an extension for PHPUnit.

@jflambert
Copy link

@jflambert jflambert commented Jan 31, 2019

Ah, we use assertArraySubset quite a lot with the current phpunit 7.5.2. Thanks for the early heads up, but could you provide some recommended alternatives?

Loading

@sebastianbergmann
Copy link
Owner Author

@sebastianbergmann sebastianbergmann commented Jan 31, 2019

For assertArraySubset() there is no suggested alternative.

Loading

@solofeed
Copy link

@solofeed solofeed commented Feb 6, 2019

@sebastianbergmann maybe will be better to improve this method or refactor instead of remove?

I think this method really useful

if I can help somehow, tell me about it

Loading

@rdohms
Copy link
Contributor

@rdohms rdohms commented Feb 15, 2019

I find it really useful to assert that an array has a set of keys/values instead of asserting one by one.

for example:

    public function fromExceptionWithDebugShouldAppendDebugInformation(): void
    {
        $dto       = ExceptionDto::fromExceptionWithDebug(new RuntimeException('Test', 123));
        $tracePart = [['function' => __FUNCTION__, 'class' => __CLASS__, 'type' => '->']];

        self::assertSame('Test', $dto->message);
        self::assertSame(RuntimeException::class, $dto->exception);
        self::assertSame(123, $dto->code);
        self::assertSame(__FILE__, $dto->file);
        self::assertSame(__LINE__ - 7, $dto->line);
        self::assertArraySubset($tracePart, $dto->trace);
        self::assertNull($dto->errorCode);
        self::assertNull($dto->violations);
    }

Loading

@maks-rafalko
Copy link

@maks-rafalko maks-rafalko commented Feb 15, 2019

We were using it a lot. Decided to copy this code to our codebase after upgrading to PHPUnit 8.0 since no alternative has been suggested.

Loading

@GrahamCampbell
Copy link
Contributor

@GrahamCampbell GrahamCampbell commented Feb 16, 2019

What is this replaced by?

Loading

@GrahamCampbell
Copy link
Contributor

@GrahamCampbell GrahamCampbell commented Feb 16, 2019

Laravel is using this too.

Loading

@rdohms
Copy link
Contributor

@rdohms rdohms commented Feb 17, 2019

As suggested by @sebastianbergmann I have moved this to a separate package and added a thin layer for ease of use. If you are in need: https://packagist.org/packages/dms/phpunit-arraysubset-asserts

Will explore what we can do to make it clearer and more flexible as well, any input is welcome.

Loading

@clicars
Copy link

@clicars clicars commented Feb 21, 2019

We use that method in every single test!! we need it!!

We are going to use the @rdohms package, if not all our tests will be destroyed.

Loading

@derrabus
Copy link
Contributor

@derrabus derrabus commented Feb 21, 2019

Well, PHPUnit 9 hasn't even been released yet and PHPUnit 8 will still be around for a while. Your tests are going to be fine.

Loading

@srosato
Copy link

@srosato srosato commented Feb 22, 2019

Removing it will break backwards compatibility, might as well take the hit and use that BC break to remove the confusion. I find this method really useful. It helps us clearly show intent on what the actual test scenario really cares about. Without that function I can see some developers reverting back to assertEquals() by copy pasting entire arrays of data that is not relevant to the test's scenario intention.

Loading

@rdohms
Copy link
Contributor

@rdohms rdohms commented Feb 22, 2019

@derrabus depends on your config, mine for example break on deprecated warning.

Loading

@derrabus
Copy link
Contributor

@derrabus derrabus commented Feb 22, 2019

@rdohms I understand. But as far as I can tell, deprecating functionality is a normal thing to do with a minor release according to SemVer. I understand that you don't want to ignore deprecations until it's too late, but your configuration sounds a bit too strict to me.

Loading

@kubawerlos
Copy link
Contributor

@kubawerlos kubawerlos commented Feb 22, 2019

@srosato

Removing it will break backwards compatibility

Thus label backward-compatibility-break ;)

Loading

@leocavalcante
Copy link

@leocavalcante leocavalcante commented Feb 23, 2019

Siler is using it as well, just got the warnings upgrading to PHPUnit 8.
It's a bummer to see it being deprecated without alternative suggestions, seems unconcerned with whoever relied on the library to write their tests.
If it was there it the first place, it provided API and people found an use case for that.

P.S.: it should be good to update the docs then: https://phpunit.readthedocs.io/en/8.0/assertions.html#assertarraysubset Add a warning telling newcomers that this feature is deprecated.

foreach ($expectedSubset as $key => $value) {
    $this->assertArrayHasKey($key, $actualArray);
    $this->assertSame($value, $actualArray[$key]);
}

Loading

@ElisDN
Copy link

@ElisDN ElisDN commented Feb 26, 2019

It is helpful for API feature tests:

public function testAuth(): void
{
   $response = $this->post('/oauth2/auth', [
       'grant_type' => 'password',
        ...
   ]);

   self::assertEquals(200, $response->getStatusCode());
   self::assertJson($content = $response->getBody()->getContents());

   $data = json_decode($content, true);

   // Check subset of all or needed static values:

   self::assertArraySubset([
       'token_type' => 'Bearer',
        ...
   ], $data);

   // Check other generated values:

   self::assertArrayHasKey('access_token', $data);
   self::assertNotEmpty($data['access_token']);
}

Loading

@rdarcy1
Copy link

@rdarcy1 rdarcy1 commented Feb 26, 2019

+1 for API tests, very useful to be able to check part of a response is present without having to check the whole thing.

Often I'll have a test for the whole response, and then separate tests to check that certain attributes change based on different setups. In these cases there's no need to check the whole response array again because it's already covered.

Loading

@module17
Copy link

@module17 module17 commented Feb 28, 2019

This one is used quite a bit. Debate the deprecation.

Loading

@deviouspk
Copy link

@deviouspk deviouspk commented Mar 5, 2019

Please do not deprecate this without a proper alternative. It's used in many projects and even big frameworks (e.g. laravel)

Loading

@Namoshek
Copy link

@Namoshek Namoshek commented Mar 10, 2019

Using this method myself in a few tests to ensure that payloads sent to and received from APIs contain the data required and also that the payloads do not contain any excess data we don't want to send.

Due to the fact that we also send data that is not persisted anywhere, we cannot perform simple string comparison of serialized JSON. Some data only lives during creation of the requests. So we need structural comparison as provided by assertArraySubset().

What does work as alternative for me is the following:

$expected = [
    'foo' => [
        'bar' => 123
    ],
    'baz' => 'hello world!'
];

$result = $someClass->myTestedMethod();

// phpunit v7
$this->assertArraySubset($expected, $result); // compare structure only
$this->assertArraySubset($expected, $result, true); // compare structure and values strictly

// phpunit v8
$this->assertTrue(
    empty(array_diff_key($expected, $result)) && empty(array_diff_key($result, $expected)
); // compare structure (keys) only
$this->assertTrue(
    empty(array_diff_assoc($expected, $result)) && empty(array_diff_assoc($result, $expected)
); // compare structure (keys) and values strictly

Actually, using array_diff_*($expected, $data) is more accurate. It will not allow for excess data in $result, which assertArraySubset() seems to do.

Loading

@MusikAnimal
Copy link

@MusikAnimal MusikAnimal commented Mar 31, 2019

If it means anything, MediaWiki (the software that powers Wikipedia) also uses assertArraySubset(), as does Symfony, and a number of related libraries and extensions. Perhaps they don't have to, just making note of it.

Loading

@ahinkle
Copy link

@ahinkle ahinkle commented Apr 4, 2019

In addition to that, I myself have yet to see a use case for this or how this would be used then. I have also not seen this used in the wild.

@sebastianbergmann This depreciation is surprising and somewhat frustrating as assertArraySubset is widely used across many frameworks and packages with no debate or alternatives.

Loading

@sebastianbergmann
Copy link
Owner Author

@sebastianbergmann sebastianbergmann commented Apr 4, 2019

All I have to say on this subject I have written down here.

If you rely on assertArraySubset() and its current behaviour and want to continue to use it then, please, go ahead, take the code, and put it into an extension. Not every assertion needs to be part of PHPUnit's standard distribution.

Loading

@deviouspk
Copy link

@deviouspk deviouspk commented Apr 4, 2019

@sebastianbergmann honestly packaging such a widely spread and basic assertion into a separate extension and thereby forcing people to rely on a less stable and maintained extension for again basic minor functionality, does not seem like a logical choice at all. Why not decide on the behavior of the method and add it as a new properly documented method? It seems like most of the frustrations are being caused when asserting a subset of a non associative array.

For most developers when they want to assert that a certain array is a subset of a non associative array. They consider the keys as something to identify the "order" of the elements in the array and not as something to identify the element by. So having one method to deal with both associative and non associative arrays didn't make sense in the first place anyway.

What would make sense is having methods like this:

assertAssocArraySubset($subset, $array, $strict=false);

assertIndexArraySubset($subset, $array, $order=false, $strict=false);

The behavior for the assertAssocArraySubset method would be exactly the same as the current assertArraySubset method.

The behavior for the assertIndexArraySubset method would differ from assertAssocArraySubset in the way that it would not consider the keys of the elements. It will also introduce a boolean order that would be false by default and checks if the elements in the subset have the same order as in the array. To illustrate this with an example i'll reference the issues encountered in #2069 .

  1. $this->assertIndexArraySubset( ['a', 'b', 'c'], ['a', 'b', 'c', 'd'] );
  2. $this->assertIndexArraySubset( ['b', 'c'], ['a', 'b', 'c', 'd'] );
  3. $this->assertIndexArraySubset( ['a', 'c', 'b'], ['a', 'b', 'c', 'd'] );
  4. $this->assertIndexArraySubset( ['a', 'c', 'b'], ['a', 'b', 'c', 'd'], true);
  5. $this->assertIndexArraySubset( ['a', 'd,' 'c'], ['a', 'b', 'c', 'd'], true);
  6. $this->assertIndexArraySubset( ['a', 'c', 'd'], ['a', 'b', 'c', 'd'], true);

Respectively this would return:

  1. true
  2. true
  3. true
  4. false
  5. false
  6. This would be the only debatable exception and the behavior for this case could be discussed. I would prefer to make it return false. Especially because you could have multiple elements with the same value. In my opinion the method should only check if the subset array is a subset and has the order of that part of the array and not look any further into the array then that.

Last but not least.
If this method is actually removed in phpunit 9 everyone will start implementing their own version of it (I doubt that there will be a stable specific extension that everyone will use to implement this functionality.). Causing a lot more confusion and frustration for people that are switching frameworks & codebases. Since behavior might differ from framework to framework. A big testing framework like phpunit has the responsability to set the standard and decide on the default behavior for basic assertions. Removing this would be a mistake.

I really hope we can give this another chance :)

Best regards,
Arthur

Loading

Repository owner locked as resolved and limited conversation to collaborators Apr 4, 2019
@sebastianbergmann
Copy link
Owner Author

@sebastianbergmann sebastianbergmann commented Apr 5, 2019

Loading

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet