Skip to content

test: Added regression test for BitWriter / BitReader bool size inconsistency #561

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.

Sign up for GitHub

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

Merged
TwoTenPvP merged 5 commits into from
Mar 12, 2021
Merged

test: Added regression test for BitWriter / BitReader bool size inconsistency #561

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
df8e587
test: Added regression test for BitWriter / BitReader bool size incon…
TwoTenPvP Mar 9, 2021
423a915
Merge branch 'develop' into test/regression-write-object-packed-bool
0xFA11 Mar 9, 2021
7a6e698
Merge branch 'develop' into test/regression-write-object-packed-bool
TwoTenPvP Mar 12, 2021
f8d5963
refactor: changed to var
TwoTenPvP Mar 12, 2021
7654332
refactor: Fixed class name
TwoTenPvP Mar 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion ...ayer.mlapi/Tests/Editor/BitStreamTests.cs → ....mlapi/Tests/Editor/NetworkBufferTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

namespace MLAPI.EditorTests
{
public class BitStreamTests
public class NetworkBufferTests
{
[Test]
public void TestEmptyStream()
Expand Down Expand Up @@ -794,5 +794,21 @@ public void TestStringPackedDiff()
Assert.That(readBuilder.ToString(), Is.EqualTo(testString));
Assert.That(stringCompare.ToString(), Is.EqualTo(testString));
}

[Test]
public void TestSerializationPipelineBool()
{
var buffer = new NetworkBuffer();
var writer = new NetworkWriter(buffer);
var reader = new NetworkReader(buffer);

writer.WriteObjectPacked(true);
writer.WriteObjectPacked(false);
Comment on lines +805 to +806

@0xFA11 0xFA11 Mar 9, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

you might add other flavors of types here such as int[], INetworkSerializable, Vector3 etc to test a few other things too.

@TwoTenPvP TwoTenPvP Mar 9, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, this PR is just for the regression of the bool bug though. But I will test all the packed ones.

@0xFA11 0xFA11 Mar 9, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

what about bool[] then? :)

@0xFA11 0xFA11 Mar 9, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ah, also Yamato fails because fix #541 went into release/0.1.0 and it's not on develop branch yet. so the test fails, which is very good! (except 2021.1 on Win passed? oh no...)

@TwoTenPvP TwoTenPvP Mar 9, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

https://yamato.cds.internal.unity3d.com/jobs/1201-Unity%20MLAPI/tree/test%252Fregression-write-object-packed-bool/.yamato%252Fproject-test.yml%2523test_project_com.unity.multiplayer.mlapi_ubuntu_trunk/5704466/job/full-pipeline

Looks failed?

@TwoTenPvP TwoTenPvP Mar 9, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

And yes, I put int he PR description: "Expect this to fail until the fix for this has been backported to develop."

@TwoTenPvP TwoTenPvP Mar 9, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yea as for yamato. The tests failed on all machine. But pack and validate succeeded. Which is a good sign!

@0xFA11 0xFA11 Mar 9, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@mattwalsh-unity was talking about merging release/0.1.0 back into develop some time soon. maybe we should that first and merge this in right afterwards?

@TwoTenPvP TwoTenPvP Mar 9, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure


buffer.BitPosition = 0;

Assert.That(reader.ReadObjectPacked(typeof(bool)), Is.EqualTo(true));
Assert.That(reader.ReadObjectPacked(typeof(bool)), Is.EqualTo(false));
}
}
}
2 changes: 1 addition & 1 deletion ...mlapi/Tests/Editor/BitStreamTests.cs.meta → ...i/Tests/Editor/NetworkBufferTests.cs.meta
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.