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

replace freezegun with time_machine #7656

Merged
merged 3 commits into from
Oct 18, 2023

Conversation

gruebel
Copy link
Contributor

@gruebel gruebel commented Oct 5, 2023

What do these changes do?

  • replaced the freezegun dependency with time-machine, because it seems like freezegun became an unmaintained project

Are there changes in behavior for the user?

Related issue number

Fixes #7655

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt
    • The format is <Name> <Surname>.
    • Please keep alphabetical order, the file is sorted by names.
  • Add a new news fragment into the CHANGES folder
    • name it <issue_id>.<type> for example (588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the pr
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

@gruebel
Copy link
Contributor Author

gruebel commented Oct 5, 2023

@Dreamsorcerer looks like time-machine doesn't support pypy 😢 Do you have a specific way on handling such cases or should it be an other library altogether?

@Dreamsorcerer
Copy link
Member

Ah, that's annoying. Not really sure what the best approach is...

@Dreamsorcerer
Copy link
Member

Dreamsorcerer commented Oct 5, 2023

Although, it looks like it's only used in 3 tests, so maybe we can just skip the tests on pypy for now...

@gruebel
Copy link
Contributor Author

gruebel commented Oct 6, 2023

definitely a possibility, but I would also need to remove the dependency from the test.in file. Should I create a separate file, like test_pypy.in with the same constraint file as test.in and then I can do an if/else block in the workflow file to use the pypy flavour only, when pypy is tested?

@Dreamsorcerer
Copy link
Member

definitely a possibility, but I would also need to remove the dependency from the test.in file.

No, look at the mypy line in that file. You can do the same kind of thing to exclude installation on pypy.

@gruebel gruebel force-pushed the replace-freezegun branch from f1ea4e2 to 5b5b0da Compare October 6, 2023 13:30
@gruebel
Copy link
Contributor Author

gruebel commented Oct 6, 2023

@Dreamsorcerer I think I added the needed changes 😅

@gruebel
Copy link
Contributor Author

gruebel commented Oct 10, 2023

@Dreamsorcerer if you have time to take a look again, it would be great 🙂

@Dreamsorcerer Dreamsorcerer added bot:chronographer:skip This PR does not need to include a change note backport-3.9 labels Oct 12, 2023
@codecov
Copy link

codecov bot commented Oct 12, 2023

Codecov Report

Merging #7656 (a8e147c) into master (2590d8d) will increase coverage by 0.00%.
Report is 1 commits behind head on master.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #7656   +/-   ##
=======================================
  Coverage   97.36%   97.36%           
=======================================
  Files         106      106           
  Lines       31648    31655    +7     
  Branches     3620     3623    +3     
=======================================
+ Hits        30813    30820    +7     
  Misses        632      632           
  Partials      203      203           
Flag Coverage Δ
CI-GHA 97.28% <100.00%> (+<0.01%) ⬆️
OS-Linux 96.95% <100.00%> (+<0.01%) ⬆️
OS-Windows 95.42% <83.33%> (-0.01%) ⬇️
OS-macOS 96.60% <83.33%> (-0.01%) ⬇️
Py-3.10.11 95.34% <83.33%> (-0.01%) ⬇️
Py-3.10.13 96.81% <83.33%> (+<0.01%) ⬆️
Py-3.11.5 96.48% <83.33%> (-0.01%) ⬇️
Py-3.8.10 95.31% <83.33%> (-0.01%) ⬇️
Py-3.8.18 96.74% <83.33%> (-0.01%) ⬇️
Py-3.9.13 95.31% <83.33%> (-0.01%) ⬇️
Py-3.9.18 96.77% <83.33%> (+<0.01%) ⬆️
Py-pypy7.3.11 96.17% <66.66%> (-0.10%) ⬇️
VM-macos 96.60% <83.33%> (-0.01%) ⬇️
VM-ubuntu 96.95% <100.00%> (+<0.01%) ⬆️
VM-windows 95.42% <83.33%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
tests/test_cookiejar.py 99.17% <100.00%> (+0.01%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@gruebel
Copy link
Contributor Author

gruebel commented Oct 18, 2023

@Dreamsorcerer anything left to do here?

@Dreamsorcerer
Copy link
Member

Sorry, just busy with 3.9 release tasks.

@Dreamsorcerer Dreamsorcerer merged commit a7bc5e9 into aio-libs:master Oct 18, 2023
@patchback
Copy link
Contributor

patchback bot commented Oct 18, 2023

Backport to 3.9: 💔 cherry-picking failed — conflicts found

❌ Failed to cleanly apply a7bc5e9 on top of patchback/backports/3.9/a7bc5e9eeae7c5c90898411962e9a74bf10a9cef/pr-7656

Backporting merged PR #7656 into master

  1. Ensure you have a local repo clone of your fork. Unless you cloned it
    from the upstream, this would be your origin remote.
  2. Make sure you have an upstream repo added as a remote too. In these
    instructions you'll refer to it by the name upstream. If you don't
    have it, here's how you can add it:
    $ git remote add upstream https://github.com/aio-libs/aiohttp.git
  3. Ensure you have the latest copy of upstream and prepare a branch
    that will hold the backported code:
    $ git fetch upstream
    $ git checkout -b patchback/backports/3.9/a7bc5e9eeae7c5c90898411962e9a74bf10a9cef/pr-7656 upstream/3.9
  4. Now, cherry-pick PR replace freezegun with time_machine #7656 contents into that branch:
    $ git cherry-pick -x a7bc5e9eeae7c5c90898411962e9a74bf10a9cef
    If it'll yell at you with something like fatal: Commit a7bc5e9eeae7c5c90898411962e9a74bf10a9cef is a merge but no -m option was given., add -m 1 as follows instead:
    $ git cherry-pick -m1 -x a7bc5e9eeae7c5c90898411962e9a74bf10a9cef
  5. At this point, you'll probably encounter some merge conflicts. You must
    resolve them in to preserve the patch from PR replace freezegun with time_machine #7656 as close to the
    original as possible.
  6. Push this branch to your fork on GitHub:
    $ git push origin patchback/backports/3.9/a7bc5e9eeae7c5c90898411962e9a74bf10a9cef/pr-7656
  7. Create a PR, ensure that the CI is green. If it's not — update it so that
    the tests and any other checks pass. This is it!
    Now relax and wait for the maintainers to process your pull request
    when they have some cycles to do reviews. Don't worry — they'll tell you if
    any improvements are necessary when the time comes!

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

@Dreamsorcerer
Copy link
Member

Dreamsorcerer commented Oct 18, 2023

If you could handle that backport, that'd be great.

@gruebel
Copy link
Contributor Author

gruebel commented Oct 20, 2023

sure, I will give it a try

@gruebel gruebel deleted the replace-freezegun branch October 20, 2023 19:08
Dreamsorcerer pushed a commit that referenced this pull request Oct 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:skip This PR does not need to include a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate to time-machine
2 participants