Skip to content

Work around uniswap and perpetual-pools ext tests failing OOM#14915

Closed
cameel wants to merge 2 commits intodevelopfrom
bump-resource-class-for-ext-tests-failing-due-to-oom
Closed

Work around uniswap and perpetual-pools ext tests failing OOM#14915
cameel wants to merge 2 commits intodevelopfrom
bump-resource-class-for-ext-tests-failing-due-to-oom

Conversation

@cameel
Copy link
Collaborator

@cameel cameel commented Mar 6, 2024

As we agreed on the call, I'm submitting my workaround as a PR, but hopefully @r0qs will come up with a better fix. Mine just massively increases the machine size to make it still work despite the huge memory leak and disables tests that fail due to some non-determinism.

@cameel cameel requested a review from r0qs March 6, 2024 14:47
@cameel cameel self-assigned this Mar 6, 2024
Comment on lines +67 to +69
# Disable a test failing due to a non-deterministic order of keys in a returned dict.
# TODO: Figure out why it's failing and re-enable.
sed -i 's|\(it\)\(("Rotates the observations array"\)|\1.skip\2|g' test/PriceObserver.spec.ts
Copy link
Collaborator Author

@cameel cameel Mar 6, 2024

Choose a reason for hiding this comment

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

Here's the relevant output from the the failed run of t_native_test_ext_perpetual_pools:

  262 passing (10m)
  15 pending
  1 failing

  1) PriceObserver
       add
         When called with a full observations array
           Rotates the observations array:

      AssertionError: expected [ …(24) ] to deeply equal [ …(24) ]
      + expected - actual

       [
         {
      +    "_hex": "0x03"
      +    "_isBigNumber": true
      +  }
      +  {
           "_hex": "0x04"
           "_isBigNumber": true
         }
         {
--
           "_hex": "0x07"
           "_isBigNumber": true
         }
         {
      +    "_hex": "0x08"
      +    "_isBigNumber": true
      +  }
      +  {
           "_hex": "0x0c"
           "_isBigNumber": true
         }
         {
      -    "_hex": "0x08"
      +    "_hex": "0x0a"
           "_isBigNumber": true
         }
         {
           "_hex": "0x0b"
           "_isBigNumber": true
         }
         {
      -    "_hex": "0x0a"
      +    "_hex": "0x0c"
           "_isBigNumber": true
         }
         {
           "_hex": "0x0e"
--
           "_hex": "0x05"
           "_isBigNumber": true
         }
         {
      -    "_hex": "0x03"
      +    "_hex": "0x05"
           "_isBigNumber": true
         }
         {
           "_hex": "0x09"
           "_isBigNumber": true
         }
         {
      -    "_hex": "0x0c"
      -    "_isBigNumber": true
      -  }
      -  {
      -    "_hex": "0x05"
      -    "_isBigNumber": true
      -  }
      -  {
           "_hex": "0x0a"
           "_isBigNumber": true
         }
         {

The dict it shows seems the have the same content, just ordered differently.

EDIT: Actually, I see now that it's a list. Still, the items are the same and I wouldn't be surprised if it was created from a dict anyway.

@cameel cameel changed the title Work around uniswap and perpetual-tools ext tests failing OOM Work around uniswap and perpetual-pools ext tests failing OOM Mar 6, 2024
Copy link
Member

@r0qs r0qs left a comment

Choose a reason for hiding this comment

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

Yeah, this will do it for now if there is no better fix. Sadly, just keeping the package.lock file doesn't worked as I expected (https://app.circleci.com/pipelines/github/ethereum/solidity/33185/workflows/5edbf286-8dab-4ee8-af98-82d25d87bf0e), so it will need some more adjusts, but at least we would not need to use xlarge resources :)

@r0qs
Copy link
Member

r0qs commented Mar 7, 2024

With this workaround here: #14919 we don't need to increase the machine sizes.

@cameel
Copy link
Collaborator Author

cameel commented Mar 7, 2024

Closing in favor of #14919.

@cameel cameel closed this Mar 7, 2024
@cameel cameel deleted the bump-resource-class-for-ext-tests-failing-due-to-oom branch March 7, 2024 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants