Skip to content

Conversation

james-toussaint
Copy link
Contributor

No description provided.

Copy link

codecov bot commented Aug 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.75%. Comparing base (934cff4) to head (9fc75d4).
Report is 8 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop     #114   +/-   ##
========================================
  Coverage    83.75%   83.75%           
========================================
  Files           34       34           
  Lines         1077     1077           
  Branches       222      222           
========================================
  Hits           902      902           
  Misses         175      175           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@james-toussaint james-toussaint marked this pull request as ready for review August 28, 2024 12:54
Copy link
Member

Choose a reason for hiding this comment

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

IexecPoco2.reveal.test.ts or IexecPoco2-reveal.test.ts ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suggest to harmonize all poco test filenames when migration is completed, are you okay with that?

Copy link
Member

Choose a reason for hiding this comment

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

No problem yes.

).to.be.revertedWithoutReason(); // require#5
});

it('Should not reveal when no able to prove result ownership', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe a better way to put it, is to contribute with another worker's valid contribution (result hash and seal of worker2).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you want I can change

            .contribute(
                 [...]
                ethers.utils.hexZeroPad('0xbad5ea1', 32), // bad seal
                 [...]
            )

to something like:

             .contribute(
                 [...]
                buildResultHashAndResultSeal(taskId, resultDigest, worker2).resultSeal // stolen result seal from another worker
                 [...]
            )

?

Copy link
Member

Choose a reason for hiding this comment

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

Yes perfect!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

).to.be.revertedWithoutReason(); // require#5
});

it('Should not reveal when no able to prove result ownership', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
it('Should not reveal when no able to prove result ownership', async () => {
it('Should not reveal when unable to prove result ownership', async () => {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I missed it thanks! Update test wording

@james-toussaint james-toussaint self-assigned this Aug 28, 2024
@james-toussaint james-toussaint merged commit 0fb4bc3 into develop Aug 28, 2024
5 checks passed
@james-toussaint james-toussaint deleted the feature/migrate-poco-reveal-unit-tests branch August 28, 2024 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants