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

Fix fifoset (3x) #905

Merged
merged 9 commits into from
Jul 10, 2019
Merged

Fix fifoset (3x) #905

merged 9 commits into from
Jul 10, 2019

Conversation

shargon
Copy link
Member

@shargon shargon commented Jul 9, 2019

Before the patch it return null values

@shargon
Copy link
Member Author

shargon commented Jul 9, 2019

We use the known hash for remove task here

image

But before the patch we get this

image

After the patch

image

So know hashes was not working as expected

@shargon shargon mentioned this pull request Jul 9, 2019
public void FifoSetTest()
{
var a = UInt256.Zero;
var b = new UInt256();
Copy link
Contributor

Choose a reason for hiding this comment

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

UInt256.Zero? Better than new UInt256(), no?

Copy link
Member Author

Choose a reason for hiding this comment

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

I want the same data, with different objects

Copy link
Contributor

@igormcoelho igormcoelho Jul 9, 2019

Choose a reason for hiding this comment

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

Better to use two new UInt256(). Isn't it clearer? Or does it risk being the same object?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, Is the same

igormcoelho
igormcoelho previously approved these changes Jul 9, 2019
Copy link
Contributor

@igormcoelho igormcoelho left a comment

Choose a reason for hiding this comment

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

Congratulations master

vncoelho
vncoelho previously approved these changes Jul 9, 2019
@igormcoelho igormcoelho added Bug Used to tag confirmed bugs Critical Issues (bugs) that need to be fixed ASAP labels Jul 9, 2019
@codecov-io
Copy link

codecov-io commented Jul 9, 2019

Codecov Report

Merging #905 into master will increase coverage by 0.13%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #905      +/-   ##
==========================================
+ Coverage   45.03%   45.16%   +0.13%     
==========================================
  Files         178      178              
  Lines       12608    12608              
==========================================
+ Hits         5678     5695      +17     
+ Misses       6930     6913      -17
Impacted Files Coverage Δ
neo/IO/Caching/FIFOSet.cs 71.42% <100%> (+48.57%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 54e772a...b0da5a6. Read the comment docs.

@vncoelho vncoelho dismissed stale reviews from igormcoelho and themself via 5547a8e July 9, 2019 18:33
@vncoelho
Copy link
Member

vncoelho commented Jul 9, 2019

@shargon and @igormcoelho, please check this additional tests for fifo size.

@vncoelho
Copy link
Member

vncoelho commented Jul 9, 2019

@shargon, there is something strangeeee..

Tests of 5547a8e should had not passed!!

@shargon
Copy link
Member Author

shargon commented Jul 9, 2019

@vncoelho it passed now

@vncoelho
Copy link
Member

vncoelho commented Jul 9, 2019

@shargon, commit 5547a8e had errors and passed, something is strange.

@vncoelho
Copy link
Member

vncoelho commented Jul 9, 2019

@shargon, I mean, this commit was wrong, it should not have passed.

image

Something weird is happening.

@igormcoelho
Copy link
Contributor

igormcoelho commented Jul 10, 2019

Strange, I'll take a look.

@shargon
Copy link
Member Author

shargon commented Jul 10, 2019

The test pass locally, please revert the changes, i will review the problem in few hours

@vncoelho
Copy link
Member

vncoelho commented Jul 10, 2019

Shargon, the problem was that all tests were passing locally, even the wrong...ajajajjashauahheuah

@shargon
Copy link
Member Author

shargon commented Jul 10, 2019

now pass, what is wrong?

erikzhang
erikzhang previously approved these changes Jul 10, 2019
@vncoelho vncoelho mentioned this pull request Jul 10, 2019
vncoelho
vncoelho previously approved these changes Jul 10, 2019
Copy link
Member

@vncoelho vncoelho left a comment

Choose a reason for hiding this comment

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

@shargon, it is very strange. I do not know why, but travis was confused here and passed a tested that failed.

I double checked everything locally and test are really failing.
Thus, I believe it is all correct.

I added some more couple of lines for ensuring Fifo max size verification.

@@ -27,6 +27,28 @@ public void FifoSetTest()
Assert.IsTrue(set.Add(c));

CollectionAssert.AreEqual(set.ToArray(), new UInt256[] { a, c });

var d = new UInt256(new byte[32] {
Copy link
Member Author

Choose a reason for hiding this comment

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

indent please

@shargon shargon merged commit 1d949c8 into neo-project:master Jul 10, 2019
@shargon shargon deleted the fix-fifoset-3x branch July 10, 2019 18:49
Thacryba pushed a commit to simplitech/neo that referenced this pull request Feb 17, 2020
Tommo-L pushed a commit to Tommo-L/neo that referenced this pull request Jun 22, 2020
* Fix fifo set

* More tests for FIFO size

* Fixing indexes

* Testing asset of different sets

* Testing again

* Update UT_FifoSet.cs

* Update UT_FifoSet.cs

* Tests for fifo max size

* Fixing indentation
lock9 added a commit to linkd-academy/neo that referenced this pull request Dec 5, 2023
lock9 added a commit to linkd-academy/neo that referenced this pull request Dec 5, 2023
lock9 added a commit to linkd-academy/neo that referenced this pull request Dec 5, 2023
lock9 added a commit to linkd-academy/neo that referenced this pull request Dec 5, 2023
Added blockchain show block/transactions/contracts commands neo-project#905 (csc…
shargon pushed a commit that referenced this pull request Dec 7, 2023
* Neo Node Migration

* Added blockchain show block/transactions/contracts commands #905 (cschuchardt88)
Added icon to applications #908 (cschuchardt88)

* Update README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Used to tag confirmed bugs Critical Issues (bugs) that need to be fixed ASAP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants