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

test(autosharding): Mocked test help #2334

Closed
wants to merge 4 commits into from

Conversation

AlejandroCabeza
Copy link
Contributor

@AlejandroCabeza AlejandroCabeza commented Jan 4, 2024

Description

Get the last test case working.
This case requires either:

  • Being able to raise a CatchableError in the system.add function.
  • Mocking.

Assuming sytem.add works flawlessly, which I kinda do; I thought the best way forward would be to mock the function but I haven't been successful in that task.
Does anyone know how to make this whole ordeal work? I'm lacking some Nim proficiency to get know my way around it.
There are a couple of issues that I'll try to address in comments. All help is welcome :)

Changes

  • Implement the catchable error on topicMap.add test case with mock

@AlejandroCabeza AlejandroCabeza added the test Issue related to the test suite with no expected consequence to production code label Jan 4, 2024
@AlejandroCabeza AlejandroCabeza changed the base branch from master to test-peer-mgmt January 4, 2024 20:34
Copy link

github-actions bot commented Jan 4, 2024

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2334

Built from eae2a2a

Copy link
Contributor Author

@AlejandroCabeza AlejandroCabeza left a comment

Choose a reason for hiding this comment

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

I've made quite a few attempts at fixing it but I've been utterly unsuccessful. Anyone has an idea on how to circumvent these errors?

The mocking mechanism used can be located here.

discard
test "catchable error on add to topicMap":
# Given the sequence.add function returns a CatchableError
let backup = system.add
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is used to keep a reference to the original function, which is necessary for cleanup later.
Produces this error: Error: invalid type: 'None' for let

test "catchable error on add to topicMap":
# Given the sequence.add function returns a CatchableError
let backup = system.add
mock(system.add):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

And then, this attempt at mocking produces:

Error: ambiguous identifier: 'add' -- you need a helper proc to disambiguate the following:
  system.add: proc (x: var seq[T], y: sink T){.noSideEffect.}
  system.add: proc (x: var string, y: string){.noSideEffect, gcsafe, locks: 0.}
  system.add: proc (x: var string, y: char){.noSideEffect.}
  system.add: proc (x: var seq[T], y: openArray[T]){.noSideEffect.}
  system.add: proc (x: var string, y: cstring){.noSideEffect, gcsafe, locks: 0.}

@AlejandroCabeza AlejandroCabeza added the help wanted Extra attention is needed label Jan 4, 2024
Base automatically changed from test-peer-mgmt to master January 5, 2024 13:49
@AlejandroCabeza AlejandroCabeza self-assigned this Jan 5, 2024
@AlejandroCabeza
Copy link
Contributor Author

Attempts

  1. let nsAdd = system.add(seq[NsContentTopic])  # Error: type mismatch: got <typedesc[seq[NsContentTopic]]
  2.  type
       NsContentTopicAdd =
         proc(x: seq[NsContentTopic], y: sink NsContentTopic) {.noSideEffect.}
    
     let 
       nsAdd: NsContentTopicAdd = system.add  # Error: type mismatch: got 'None' for 'add' but expected 'NsContentTopicAdd = proc (x: seq[NsContentTopic], y: NsContentTopic){.closure, noSideEffect.}

Notes

Attempt 1

Calling add like so doesn't match, because the expected first parameter is a type instance, not a type.

Attempt 2

One of the possible disambiguations shown by the compiler is the following:

Error: ambiguous identifier: 'add' -- you need a helper proc to disambiguate the following:
  ...
  system.add: proc (x: var seq[T], y: sink T){.noSideEffect.}
  ...

When following the same pattern as above, but with a different disambiguation, the compiler doesn't complain:

type
  NsContentTopicSeqAdd =
    proc(x: var seq[NsContentTopic], y: openArray[NsContentTopic]) {.
      noSideEffect
    .}

let
  nsSeqAdd: NsContentTopicSeqAdd = system.add
  nsSeqAddUnsafeAddr = unsafeAddr(nsSeqAdd)

echo repr(nsSeqAdd)  # [Field0 = 0x55dc4e228490, Field1 = nil]
echo repr(nsSeqAddUnsafeAddr)  # ptr 0x7ffc0384d030 --> [Field0 = 0x55dc4e228490, Field1 = nil]

I've also tried running mock with the system.add: NsContentTopicSeqAdd but I get to a SEGFAULT.

@AlejandroCabeza
Copy link
Contributor Author

Posted question to Nim Forum: Link

@AlejandroCabeza AlejandroCabeza deleted the test-autosharding-mock branch January 25, 2024 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed test Issue related to the test suite with no expected consequence to production code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant