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

tests: Fix `/status' endpoint to cater for lists #1371

Closed
wants to merge 1 commit into from

Conversation

ac000
Copy link
Member

@ac000 ac000 commented Jul 25, 2024

We can now get list objects from the /status endpoint in the case of
having different versions of the same language module.

That led to this error

  >       return d1 - d2
  E       TypeError: unsupported operand type(s) for -: 'list' and 'list'

We already cover a similar case for when we have simple strings so add
this to that.

@andrey-zelenkov
Copy link
Contributor

Previously, there were no lists in /status. Once "modules" were implemented, it became possible to get a list by compiling multiple versions of the same language module. Buildbot triggers this issue by compiling multiple Java modules:

{
  "modules": {
  ...
    "java": [
      {
        "version": "18.0.2-ea",
        "lib": "/path/to/unit/modules/java-18.unit.so"
      },
      {
        "version": "17.0.10",
        "lib": "/path/to/unit/modules/java-17.unit.so"
      },
      {
        "version": "11.0.22",
        "lib": "/path/to/unit/modules/java-11.unit.so"
      }
    ]
  }
  ...
}

@ac000
Copy link
Member Author

ac000 commented Jul 26, 2024

Previously, there were no lists in /status. Once "modules" were implemented, it became possible to get a list by

This seems to effect at least Python 3.6 and 3.7. I don't have this issue with Python 3.12 (tested with multiple wasm modules), as it's coming through as a dict, I assume anyway, perhaps I should double check that and it's not that newer python supports - on lists.

compiling multiple versions of the same language module. Buildbot triggers this issue by compiling multiple Java modules:

It's when having Python 3.x and 2.x modules built that I'm seeing it fail on RHEL 8 and AmazonLinux 2. But yes...

@andrey-zelenkov
Copy link
Contributor

This seems to effect at least Python 3.6 and 3.7. I don't have this issue with Python 3.12 (tested with multiple wasm modules), as it's coming through as a dict, I assume anyway, perhaps I should double check that and it's not that newer python supports - on lists.

A few Python versions from random buildbot VMs that reproduce the issue:

platform linux -- Python 3.7.16, pytest-7.4.2,
platform linux -- Python 3.9.2, pytest-8.2.1,
platform linux -- Python 3.10.12, pytest-7.4.2,
platform linux -- Python 3.12.0, pytest-7.3.2,

@ac000
Copy link
Member Author

ac000 commented Jul 26, 2024

...
platform linux -- Python 3.12.0, pytest-7.3.2,

Weird, I don't see this issue with Python 3.12 on Fedora 40...

Actually I do...

@ac000
Copy link
Member Author

ac000 commented Jul 28, 2024

Fix commit message

$ git range-diff a5de03fd...fd26c459
1:  a5de03fd ! 1:  fd26c459 tests: Fix `/status' endpoint tests for Python 3.6
    @@ Metadata
     Author: Andrew Clayton <[email protected]>
     
      ## Commit message ##
    -    tests: Fix `/status' endpoint tests for Python 3.6
    +    tests: Fix `/status' endpoint to cater for lists
     
    -    When there is an array of different versions of the same language
    -    module, it seems at least Python 3.6 sees this as a list rather then a
    -    dict as later versions see it and so we get the following error
    +    We can now get list objects from the /status endpoint in the case of
    +    having different versions of the same language module.
    +
    +    That led to this error
     
           >       return d1 - d2
           E       TypeError: unsupported operand type(s) for -: 'list' and 'list'

@andrey-zelenkov
Copy link
Contributor

Side note: The order of elements in lists is important when comparing them ([1,2] == [2,1] // False). Therefore, it might be a good idea to sort list elements before comparing them (e.g., by some key values or serialized versions in the case of dictionaries) if the exact same order is not guaranteed.

@ac000
Copy link
Member Author

ac000 commented Jul 29, 2024

Where do these two lists even come from?

@andrey-zelenkov
Copy link
Contributor

Both lists come from GET requests to /status or /status/path/to/metric at different times. The first GET request occurs in Status.init() and stores the result in Status._status, while the second GET request happens during the Status.get() call.

The reason Status._status exists is that tests do not restart Unit between runs, and there is no way to flush /status values. As a result, the /status accumulate values between test executions. Therefore, it is necessary to set a "zero level" before each test.

@ac000
Copy link
Member Author

ac000 commented Aug 7, 2024

Side note: The order of elements in lists is important when comparing them ([1,2] == [2,1] // False). Therefore, it might be a good idea to sort list elements before comparing them (e.g., by some key values or serialized versions in the case of dictionaries) if the exact same order is not guaranteed.

Hmm, of course, they are already sorted by Unit... (by module name, then by version)

@ac000 ac000 changed the title tests: Fix `/status' endpoint tests for Python 3.6 tests: Fix `/status' endpoint to cater for lists Aug 15, 2024
@callahad callahad self-assigned this Aug 19, 2024
@callahad callahad added this to the 1.33 milestone Aug 19, 2024
@ac000
Copy link
Member Author

ac000 commented Aug 28, 2024

Just to clarify, I'm happy with this patch, whether it's 100% the right thing or not, it solves the immediate issue and merely extends on a previous fix for a similar issue...

@ac000 ac000 mentioned this pull request Aug 28, 2024
6 tasks
We can now get list objects from the /status endpoint in the case of
having different versions of the same language module.

That led to this error

  >       return d1 - d2
  E       TypeError: unsupported operand type(s) for -: 'list' and 'list'

We already cover a similar case for when we have simple strings so add
this to that.

Signed-off-by: Andrew Clayton <[email protected]>
@ac000
Copy link
Member Author

ac000 commented Aug 28, 2024

Rebase with master

$ git range-diff fd26c459...f4298f94
 -:  -------- >  1:  57a75ea0 build(deps): bump openssl from 0.10.64 to 0.10.66 in /tools/unitctl
 -:  -------- >  2:  b892e994 tools/unitctl: update readme
 -:  -------- >  3:  1b484303 tools/unitctl: update readme
 -:  -------- >  4:  e743b6ce tools/unitctl: remove (default) from option text
 -:  -------- >  5:  2a243740 tools/unitctl: make json-pretty default output fmt
 -:  -------- >  6:  43faf99d tools/unitctl: reword freeform message for output
 -:  -------- >  7:  a91b961d tools/unitctl: make application directory configurable
 -:  -------- >  8:  e56c4ede fuzzing: code cleanup
 -:  -------- >  9:  900d25c3 fuzzing: fixed harness bug
 -:  -------- > 10:  bc49274d fuzzing: updated JSON target
 -:  -------- > 11:  3667c3e2 fuzzing: added new basic targets
 -:  -------- > 12:  06c4ea1f Add a basic .editorconfig file
 -:  -------- > 13:  0f313e2b CONTRIBUTING.md: Re-flow text
 -:  -------- > 14:  20224279 CONTRIBUTING.md: Update the 'Git Style Guide' section
 -:  -------- > 15:  5d32e500 Packaging: fix build-depends on multiarch debian systems
 -:  -------- > 16:  12c376ab README: Update number of supported languages
 -:  -------- > 17:  11a70a32 docs/openapi: Update the /status endpoint URL
 -:  -------- > 18:  ae4795aa docs/openapi: Add entries for the new /status/modules endpoint
 -:  -------- > 19:  f38201c2 auto: Add a check for Linux's sched_getaffinity(2)
 -:  -------- > 20:  2444d45e lib: Better available cpu count determination on Linux
 -:  -------- > 21:  57c88fd4 router: Make the number of router threads configurable
 -:  -------- > 22:  97c15fa3 socket: Use a default listen backlog of -1 on Linux
 -:  -------- > 23:  76489fb7 conf, router: Make the listen(2) backlog configurable
 -:  -------- > 24:  2eecee75 var: Restrict nxt_tstr_query() to only support synchronous operation
 -:  -------- > 25:  76a255b2 http: Refactor return action
 -:  -------- > 26:  08a23272 http: Refactor route pass query
 -:  -------- > 27:  9d19e7e0 http: Refactor static action
 -:  -------- > 28:  ecb3f86c http: Refactor access log write
 -:  -------- > 29:  5f6ae1a1 var: Remove unused functions and structure fields
 -:  -------- > 30:  82e168fe http: Refactor out nxt_tstr_cond_t from the access log module
 -:  -------- > 31:  57f93956 http: Get rid of nxt_http_request_access_log()
 -:  -------- > 32:  debd61c3 http: Add "if" option to the "match" object
 -:  -------- > 33:  43c4bfdc tests: "if" option in http route match
 -:  -------- > 34:  593564fd ci/unitctl: Update paths
 -:  -------- > 35:  cad6aed5 Tests: initial "wasm-wasi-component" test
 -:  -------- > 36:  05b1c769 docs/openapi: Fix brokenness
 -:  -------- > 37:  cff18f89 docs/openapi: Add new config options
 -:  -------- > 38:  71920769 fuzzing: fixed harness bug
 -:  -------- > 39:  932b9146 socket: Prevent buffer under-read in nxt_inet_addr()
 -:  -------- > 40:  1a685084 Docker: bump Go versions
 -:  -------- > 41:  5b47542e Docker: update Rust version
 -:  -------- > 42:  8eb5d128 Docker: introduce "slim" python images
 -:  -------- > 43:  4778099b Docker: leave artifacts when build targets succeed
 -:  -------- > 44:  6e3152c0 Remove .hgtags
 -:  -------- > 45:  778d81cc Remove .hgignore files
 -:  -------- > 46:  0951778d Added .gitignore for pkg/contrib/tarballs
 1:  fd26c459 = 47:  f4298f94 tests: Fix `/status' endpoint to cater for lists

@ac000 ac000 closed this Aug 28, 2024
@ac000 ac000 deleted the pyetst-fix branch August 28, 2024 21:16
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.

4 participants