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

Pin bottle version to fix CI runs #542

Merged
merged 1 commit into from
Sep 11, 2024
Merged

Conversation

glatterf42
Copy link
Member

@glatterf42 glatterf42 commented Sep 10, 2024

Exactly the same as #541 to ensure the windows CI failures don't originate from that PR coming from my fork.

Four days ago, bottle released version 0.13.0 and v0.13.1 two days afterwards. Unfortunately, these versions are incompatible with pretenders, which we use in test_access to mock a local http server which we can target with access-related requests.

Pretenders hasn't been updated in two years, so I think it's unlikely we'll receive a patch here. For clarification, I've still opened pretenders/pretenders#153, though.

To replace pretenders, I thought about using monkeypatch to patch the result of self._backend.get_auth() in Platform.check_access(), but that would probably miss the point since we want to test this authentication-getting function, I think.
I've considered responses (as suggested here) but couldn't quite get it to work. Maybe the information I'm missing is not in their docs or maybe it's just too awkward to configure a mock server response for the authentication code, which seems to be baked into the old java code.
Next, I've tried pytest-httpserver, which just needs to be installed with pip and is then immediately available as a pytest fixture. I got a little further here, did at least manage to get the login request sent to the mock server, but still couldn't make it work because I was apparently missing something about json-deserialization, strings and lists in java:

Caused by: com.fasterxml.jackson.databind.exc.MismatchedInputException: Cannot deserialize instance of `java.util.ArrayList` out of VALUE_STRING token

Lastly, I considered migrating the code from pretenders into our code base (the essential part at least) and using bottle directly. However, this seems unwise in terms of maintenance. So for now, this PR pins the bottle version to the latest working one. In an ideal world, we put a bit more focus on developing ixmp4 and begin the migration still this year, so that we don't have to worry about this for too long.
Alternatively, we might have to revisit this and fix our mocking approach when the pinned bottle version doesn't support the latest python version or some such.

How to review

Please jump in here if you have a good idea for a sustainable solution. Otherwise, just note the CI checks all pass ... for now.

@khaeru Please let me know if we should also put this pin in pyproject.toml. If we don't, new installations will probably end up with not-working dependencies.

PR checklist

  • Continuous integration checks all ✅
  • Add or expand tests; coverage checks both ✅ Just CI.
  • Add, expand, or update documentation. Just CI.
  • Update release notes.

@glatterf42 glatterf42 added the bug label Sep 10, 2024
@glatterf42 glatterf42 self-assigned this Sep 10, 2024
@glatterf42 glatterf42 added backend.jdbc Interaction with ixmp_source via JDBCBackend & JPype dependencies Pull requests that update a dependency file labels Sep 10, 2024
@glatterf42 glatterf42 requested a review from khaeru September 10, 2024 06:35
@glatterf42
Copy link
Member Author

It looks like the windows CI failures did indeed arise from #541 coming from my fork. Apparently, the macos and linux runners were able to use caches and access the GAMS_LICENSE through that, too, while the windows runners could not do that and crashed.

@glatterf42 glatterf42 mentioned this pull request Sep 10, 2024
2 tasks
Copy link
Member

@khaeru khaeru left a comment

Choose a reason for hiding this comment

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

Thanks for the diagnosis, upstream bug filed, and fix—I had noticed these failures last week, but did not find time to attribute to a bottle release and its incompatibility with pretenders.

I think this is the correct response, and we should wait/consider whether we need a pin in pyproject.toml. I think pretenders per se is only used in the test suite. The code it is used to test is rarely used. So even users try to run the test suite with latest bottle and fail, they should still be able to use the features that are tested.

In terms of alternatives to pretenders, I have also used requests-mock and I sense it is actively maintained. But I do not know if it would work in this case (esp. if the underlying HTTP activity is happening in the Java code).

@khaeru khaeru added the ci Continuous integration label Sep 11, 2024
@khaeru khaeru merged commit 90cfe46 into main Sep 11, 2024
59 of 74 checks passed
@khaeru khaeru deleted the pin/bottle-pretenders-breakage branch September 11, 2024 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend.jdbc Interaction with ixmp_source via JDBCBackend & JPype bug ci Continuous integration dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants