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

Tests on Windows + GitHub Actions #314

Merged
merged 34 commits into from
Nov 13, 2023

Conversation

elliotwutingfeng
Copy link
Contributor

Tests on Windows via GitHub Actions were not working. While trying to fix them, I've uncovered some strange behaviors. Currently all tests pass on macOS, Windows, and Linux for python 3.8-3.12 + pypy3.8.

1. Incomplete path conversion on Windows

import os
from pathlib import Path, PureWindowsPath

os.path.join('/home/anonymous', 'apple')
'/home/anonymous\\apple'

str(Path('/home/anonymous','apple'))
# you can simulate windows style Path() on UNIX systems via str(PureWindowsPath('/home/anonymous','apple'))
'\\home\\anonymous\\apple'

Unlike str(pathlib.Path), os.path.join does not convert existing forward-slashes, causing incomplete conversion of the monkeypatched UNIX-style paths to Windows-style.

  monkeypatch.setenv("HOME", "/home/john")
  monkeypatch.setenv("XDG_CACHE_HOME", "/my/alt/cache")
  monkeypatch.delenv("TLDEXTRACT_CACHE", raising=False)

2. Python 3.11 and 3.12 on Windows

For some unknown reason, in Python 3.11 & 3.12 on Windows (but not UNIX), the following receives a WindowsPath instead of a string. So I modified evil_unlink to perform relative path checks for both string and Path (since os.unlink supports both datatypes).

def evil_unlink(filename: str) -> None:

3. Erroneous stdout on pypy3.9 and pypy3.10 (all OSes)

See https://github.com/elliotwutingfeng/tldextract/actions/runs/6810665611/job/18519409296#step:5:60

E       AssertionError: assert '/Users/runne...s bbc co.uk\n' == ' example com...s bbc co.uk\n'
E         + /Users/runner/work/tldextract/tldextract/.tox/pypy39/lib/pypy3.9/site-packages/certifi/cacert.pem None
E            example com
E            bbc co.uk
E           forums bbc co.uk

I believe this has to do with the requests library, but I am unable to suppress the output (some kind of tuple?). This is probably the most important issue since pypy only supports the latest 1 or 2 versions of pypy.


4. Consider switching to GitHub Actions from Travis

If you are spending money on Travis, you may want to consider switching to GitHub Actions as it is free for public projects like this.


Copy link
Owner

@john-kurkowski john-kurkowski left a comment

Choose a reason for hiding this comment

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

Hey! This PR makes sense. Love the path fixes.

As for CI, I was not committed to Travis CI. I was on their free tier for OSS. However, I have been using GitHub Actions in other projects, and love the integration and community. Happy to switch.

Before we merge, I would love to see a GitHub Actions checkmark on this PR. Not sure why it's not running.

tests/test_parallel.py Outdated Show resolved Hide resolved
@john-kurkowski
Copy link
Owner

Okay there's a green checkmark on eb99020. 👍

@john-kurkowski john-kurkowski merged commit 274b0b2 into john-kurkowski:master Nov 13, 2023
21 checks passed
@elliotwutingfeng elliotwutingfeng deleted the gha branch November 13, 2023 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants