-
Notifications
You must be signed in to change notification settings - Fork 0
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
Actually update the dependencies in updated_tests.yml #38
Actually update the dependencies in updated_tests.yml #38
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #38 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 2 2
Lines 7 7
=========================================
Hits 7 7 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why this poetry lock
would change anything here. I thought this is just necessary when there's no lock file around, or when you want to react to a change in the toml file. We do have the lock file in the repos, so poetry install
will install from those and then poetry update
will change that lock file and install any dependencies that were updated. What am I misunderstanding? Or maybe we should just put the update command here before the poetry install
?
Hmm yes you are right, this shouldn't matter. But why then did the CI not fail on these I doubt this PR would fix that. (That is, would cause the CI to fail.) Maybe we should revert AstarVienna/ScopeSim#504 (or create experimental branch from before that was merged) and see whether we can get the CI to fail. |
Yeah, just branch off from before and run the CI on that. Should even work locally by checking out an earlier commit and running |
Yeah it works locally. Just not on the CI. But now I see why: these are all webtests that fail (duh, xml parsing), and those are not tested with the updated dependencies. But the webtests will be tested with updated dependencies once AstarVienna/ScopeSim_Data#20 is merged! (I should just use I'll close this, thanks for catching my error. |
Reopening this, because |
I suppose we could also instead do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it works (better) that way, let's just do this. If I ever find time to investigate what would be the proper way to do this with poetry, I can always go back and change it...
I created a little test project: https://github.com/hugobuddel/poetrytest |
The goal of the updated tests was to run a project with the latest version of its dependencies, so we would discover breaking changes and/or deprecations early, so we don't "suddenly" have to fix those.
However, we did not actually update the dependencies. This rectifies that.
Note that this still does not actually test any dependency that has a newer 'major' version change.
E.g. see https://github.com/AstarVienna/ScopeSim_Data/actions/runs/11878297751/job/33098809058 , apparently "The 'strip_cdata' option of HTMLParser() has never done anything and will eventually be removed." Now this is something we can just ignore, as we don't directly use that option ourselves.
In fact, I believe this problem has been fixed in beautifulsoup 4.13.0b2, so apparently this change also does not update to beta releases.