Fix UT asserts

[PierrickVoulet]

Asserts always return true when comparing two FieldMask instances. I switch to a comparison of their JSON string representation instead.

KNOWN ISSUE: 2 UTs fail with this new way of assertion, I mark this PR as DRAFT for now until a fix is found.

Change-Id: I3b890a3bdcceb21876b157eda96d066938638387

[PierrickVoulet]

@fiboknacky I see you changed some UTs in #291, any further context you could provide before I deep dive into the FieldMarks util class by any chance?

[fiboknacky]

@fiboknacky I see you changed some UTs in #291, any further context you could provide before I deep dive into the FieldMarks util class by any chance?

I'm not sure what in particular you would like me to provide more. Could you elaborate?
I think all the context was discussed and given during the review process.

To provide a summary here again, the PR was to fix the wrong behavior of FieldMasks. This makes the PHP lib aligned with the Java lib as well.
I'd recommend comparing the "before" and "after" versions of test_cases.json. That would show what specific cases we wanted to fix.

[fiboknacky]

Asserts always return true when comparing two FieldMask instances. I switch to a comparison of their JSON string representation instead.

What do you mean by this? Do you mean testFieldMaskCompare always passed before?

[PierrickVoulet]

Asserts always return true when comparing two FieldMask instances. I switch to a comparison of their JSON string representation instead.

What do you mean by this? Do you mean testFieldMaskCompare always passed before?

When I assert that two FieldMask instance values are equal, it always returns true even when they are not equal.

<?php
// ...
$this->assertEquals($expectedFieldMask, $actualFieldMask);

When I change the values to their JSON strings instead, the comparison then returns false when expected in our tests. This works fine because each of our test has either 0 or 1 path in the expected FieldMask, I am not sure how it would react with > 1 paths though (the order of elements could be unstable at runtime).

<?php
// ...
$this->assertEquals(
  $expectedFieldMask->serializeToJsonString(),
  $actualFieldMask->serializeToJsonString()
);

[fiboknacky]

Asserts always return true when comparing two FieldMask instances. I switch to a comparison of their JSON string representation instead.

What do you mean by this? Do you mean testFieldMaskCompare always passed before?

When I assert that two FieldMask instance values are equal, it always returns true even when they are not equal.

<?php
// ...
$this->assertEquals($expectedFieldMask, $actualFieldMask);

What cases that you expected to see it failed but it succeeded?

When I change the values to their JSON strings instead, the comparison then returns false when expected in our tests. This works fine because each of our test has either 0 or 1 path in the expected FieldMask, I am not sure how it would react with > 1 paths though (the order of elements could be unstable at runtime).

<?php
// ...
$this->assertEquals(
  $expectedFieldMask->serializeToJsonString(),
  $actualFieldMask->serializeToJsonString()
);

I think this is a bit not quite correct. We shouldn't depend on how the fields are serialized as string, unless the serializeToJsonString always serialize fields in a deterministic way.

[PierrickVoulet]

@fiboknacky sorry I forgot to flag the PR as draft, this code is exploratory. I added you as reviewer but I only wanted to gather historical context about these tests.

Summary
This PR aims to fix two distinct issues:

1. The implementation of the FieldMask UTs which does not work as expected: the asserts always return true even when they should return false
2. Fixing the implementation of the FieldMask UTs, I noticed that two UTs that are related to repeated field modification fail.

Details
I updated the code to better highlight these two issues we are facing. Here are the two UTs that do not fail using the current implementation but do when using my not-perfect-but-working implementation that compares the paths as JSON strings instead.

============ TEST 'Modify element in a repeated field'
- expected: foos 
- actual: 
Testing with FieldMask instances...
Testing with FieldMask JSON string...
Failed asserting that two strings are equal.
Expected :'"foos"'
Actual   :'""'

============ TEST 'Modify element in a nested repeated field'
- expected: foos 
- actual: 
Testing with FieldMask instances...
Testing with FieldMask JSON string...
Failed asserting that two strings are equal.
Expected :'"foos"'
Actual   :'""'

[fiboknacky]

Understood. Please let me know when it's ready for review.

[PierrickVoulet]

Quick update: after further investigation the UTs fail due to wrong comparison of repeated fields. I opened the issue haberman/protobuf#13 to confirm.

[PierrickVoulet]

The current behavior of repeated field is a bug, fix should come from this protocolbuffers/protobuf#7650.

[PierrickVoulet]

I just tested with protocolbuffers/protobuf#7650 and protocolbuffers/protobuf#7900 and it seems to be fixed. Let's wait for the next protobuf release before closing this though.

[PierrickVoulet]

I confirm that all the unit tests pass successfully with the new release of protobuf, I am closing this because it is fixed.