-
Notifications
You must be signed in to change notification settings - Fork 4
PR for #137: test new python versions #138
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
Conversation
|
The changes to this pull ended up being more substantial than I originally anticipated. Below, I describe what happened and the fix I made. Our lab template has different unit tests for windows and unix operating systems. In the past, to get windows unit tests to pass on an unix OS, we would "pretend" to be on a windows platform for those tests. That was the purpose of the At a high level, as of Python 3.12, that no longer works, because the behavior of windows commands on a windows system no longer aligns with actual windows commands on a linux system pretending to be a windows platform. Hence, in addition to updating the python versions the template officially supports, I had to modify our unit tests slightly. Technical detailsPython 3.12 included this PR (python/cpython#103179), which modified the The least intrusive change would be to add this attribute to the Windows platform API objects for unix OS's. However, it seems a little odd to pretend to be a windows platform on a unix OS (and vice versa) for the sake of passing unit tests, instead of just skipping the unit tests that aren't relevant to the OS. In this pull, I updated the code to do just that - we only run windows-specific unit tests on windows OS's and the same for unix etc. If we look at the On an unrelated note, this was fun to learn about. |
jmshapir
left a comment
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.
@liaochris looks good to me!
@jinfu93 would you be up for taking a quick pass over the changes to confirm they look ok to you too?
Thanks both!
|
@liaochris @jmshapir reviewed and changes make sense to me too! |
Opening pull so that github actions will run. Will assign reviewers once I confirm that tests pass.