Skip to content

Conversation

@Eta0
Copy link
Collaborator

@Eta0 Eta0 commented May 10, 2025

Disambiguate URL parsing in a unit test

This change fixes a unit test that was incorrectly failing. It was essentially verifying that the string X-Amz-Expires was present in a URL querystring, but that Expires wasn't, but didn't properly account for Expires being a substring of X-Amz-Expires.
It was slightly more nuanced than this, so it didn't fail 100% of the time. This changes the regex so that the search for the substring Expires does not match if it is preceded by the substring X-Amz-.

This only fixes a unit test and does not require any updated release or changelog notes.

@Eta0 Eta0 requested a review from wbrown May 10, 2025 01:44
@Eta0 Eta0 self-assigned this May 10, 2025
@Eta0 Eta0 added the bug Something isn't working label May 10, 2025
@Eta0 Eta0 requested a review from sangstar May 23, 2025 14:43
Copy link
Contributor

@sangstar sangstar left a comment

Choose a reason for hiding this comment

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

LGTM!

]
V2_regex = [
r"Expires=1[0-9]+",
r"(?<!X-Amz-)Expires=1[0-9]+",
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason why you hardcode the 1 here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure why that's there. It was added by someone else without any explanation that I found, so I left it.

@Eta0 Eta0 merged commit 9241bc8 into main May 23, 2025
2 checks passed
@Eta0 Eta0 deleted the eta/fix-test-regex branch May 23, 2025 17:54
maresb added a commit to regro-cf-autotick-bot/tensorizer-feedstock that referenced this pull request Jun 14, 2025
Error: 
  × missing field `script`
    ╭─[recipe/recipe.yaml:39:10]
 38 │           python_version: ${{ python_min }}.*
 39 │ ╭─▶   - files:
 40 │ │         source:
 41 │ │           - tests/
 42 │ │       requirements:
 43 │ │         run:
 44 │ │           - pytest
 45 │ │           - pytest-rerunfailures
 46 │ │           - transformers >=4.27.1
 47 │ │           - moto >=4.1.4,<5.0.0
 48 │ │           - redis-py >=5.0.0
 49 │ │           - if: not win
 50 │ │             then:
 51 │ │               - redis-server
 52 │ │           - boto3
 53 │ │       script:
 54 │ │       # Redis tests are flaky, sometimes failing with:
 55 │ │       #     redis.exceptions.ConnectionError: Error 111 connecting to localhost:6381.
 56 │ │       #     Connection refused.
 57 │ │       # I suspect that there's not enough startup time, so we retry a few times.
 58 │ │         - if: linux
 59 │ │           then:
 60 │ │             # Skip test_download_url until <coreweave/tensorizer#188> is merged.
 61 │ │             - pytest tests -vvv --reruns 2 --deselect tests/test_stream_io.py::TestS3::test_download_url
 62 │ │   
 63 │ ├─▶ about:
    · ╰──── here
 64 │       homepage: https://github.com/coreweave/tensorizer
    ╰────
  help: expected field `script` to be a list of commands
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants