General improvements, and the UnitTests add-on #5
Conversation
Fix deserialization of a single two-dimensional array and hashtable. Add and updated UnitTest.
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (15)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
/azp run pipeline |
|
No pipelines are associated with this pull request. |
|
@RelaxSpirit the issue with triggering the pipeline seems to persist... could it be that you're still using the original repository and that is causing git to fail to handle this? If that's the case, any chance that you can fork from this one, open a new branch and copy over your file so it picks up the changes? |
|
@josesimoes Strange. I made a new fork to the original repository for myself. After the first successful PR, I synchronized my fork with the current version of main. After that, I made an unnecessary memory_allocations brunch in my fork. It was at this brunch that I made the changes, and it was he who went into PR. Maybe there is some omission in my actions?
|
|
Maybe this open PR is getting in the way, and I need to close it? |
|
/azp run pipeline |
|
No pipelines are associated with this pull request. |
|
/azp run pipeline |
|
No pipelines are associated with this pull request. |
|
@RelaxSpirit this may be fixed now. Don't worry, the other PR should have no interference with this. In the meantime, the PR has failed with failure in 2 unit tests. Care to check please? |
|
/azp run pipeline |
|
No pipelines are associated with this pull request. |
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
@josesimoes Please forgive me, I'm not very familiar with all the project assembly pipelines yet. Could you please provide a link to the unit test error in the pipeline. I only found build errors - Added comment I can't find any unit test errors :( Before delivering the code, I always check the completion of all unit tests: |
|
No problem, here it is. 
View Details: 
Build job and then scroll down until you see the failing task. In this case, the one running unit tests: 
|
|
@josesimoes But from your screenshots, I understood where the unit test might fall :) long[][] twoDimensionalArray = new long[][]
{
new long[]{1, 2, 3, 4, 5},
new long[]{long.MaxValue -1, -1, -3985948353983, -8923879},
new long[]{8734832748327, -84393275329, 87348932758, long.MinValue},
};
arrayBytes = MessagePackSerializer.Serialize(twoDimensionalArray);
var twoDimensionalArrayValue = (long[][])MessagePackSerializer.Deserialize(typeof(long[][]), arrayBytes)!;
Assert.IsTrue(twoDimensionalArray.ArrayEqual(twoDimensionalArrayValue));It looks like running this procedure twoDimensionalArray.ArrayEqual(twoDimensionalArrayValue) does not give the same results depending on the runtime environment. |
|
If you navigate upper in the pipeline window you'll get to the next build run and find the one failing at the unit tests task. Anyway, the runner in the pipeline is supposed to be the same that it's running locally. Except if you are running it on a real device... Know that there are asserts for collections in our test framework. You may want to use those for the array... |
|
@josesimoes Thank you. With a direct link, I figured out where the build was lost :) |
|
/AzurePipelines run |
|
Commenter does not have sufficient privileges for PR 5 in repo nanoframework/nanoFramework.MessagePack |
RelaxSpirit
left a comment
RelaxSpirit
left a comment
There was a problem hiding this comment.
According to the comments, there will be corrections for the files of this PR. Throughout the solution code, corrections based on comments will be in a separate PR.
|
@RelaxSpirit OK to add the corrections here, otherwise it will require to read through the code again to check those. |
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@josesimoes The build dropped again on the tests. But now it's quite clear what's wrong :) |
|
That's a question to ask, definitely. What is the nanoCLR version that you see locally?
|
|
Test run detected DLL(s) which would use different framework and platform versions. Following DLL(s) do not match current settings, which are .NETFramework,Version=v4.8 framework and X64 platform. |
|
Just missing one line where the nanoCLR version is printed... |
|
@josesimoes There is an entry - [nanoTestAdapter]: *** Failed to update nanoCLR instance *** I'm not allowed direct access :) After connecting the VPN - [nanoTestAdapter]: Getting ready to run tests... Loading nanoCLR v1.12.4.131 ========== Test run finished: 5 Tests (5 Passed, 0 Failed, 0 Skipped) run in 7,2 sec ========== |
|
@josesimoes Figured it out. The behavior and operation of the code differs between Release and Debug :) [nanoTestAdapter]: Getting ready to run tests... ========== Test run finished: 5 Tests (4 Passed, 1 Failed, 0 Skipped) run in 3,1 sec ========== That is, the problem is reproduced locally. That's good :) |
|
Yes, nanoCLR versions are the same in both runs. |
|
@josesimoes In the code nanoFramework.MessagePack there is no conditional compilation for RELEASE and DEBUG. |
|
@josesimoes I fixed the bug with the unit test. To deliver changes to this PR again? |
|
Yes please. Curious on what was there. |
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
|
@RelaxSpirit thank you again for your contribution! 🙏😄 .NET nanoFramework is all about community involvement, and no contribution is too small. Please edit it and add an entry with your GitHub username in the appropriate location (names are sorted alphabetically): (Feel free to adjust your name if it's not correct) |
3 participants
Description
Removed unnecessary memory allocations.
Fix deserialization of a single two-dimensional array and hashtable.
Updated UnitTest
Motivation and Context
Memory usage is not optimal during deserialization.
Insufficient coverage of the code by unit tests
How Has This Been Tested?
UnutTest.
Types of changes
Checklist: