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

refactor test setups towards fixtures and hinting #968

Merged
merged 17 commits into from
Mar 15, 2023

Conversation

altendky
Copy link
Contributor

@altendky altendky commented Mar 14, 2023

This is an attempt to switch to well hinted pytest fixtures that mypy and flake8 will understand with as little change to the actual tests as possible. This does not necessarily represent the pattern I might normally apply. That can come later, if at all.

Draft for:

  • feedback on the direction
  • application to other setup fixtures

@altendky altendky marked this pull request as draft March 14, 2023 14:07
@BoboTiG
Copy link
Collaborator

BoboTiG commented Mar 14, 2023

Well, that's a great job :)
Could we move all those stuff (fixtures + mypy-specific code) to conftest.py?

@BoboTiG
Copy link
Collaborator

BoboTiG commented Mar 14, 2023

FTR if it's too time consuming, it's also OK to skip Mypy in tests. It's up to you :)

@altendky
Copy link
Contributor Author

Yes, they are expected to move to conftest.py to be shared. I personally very much like to have tests fully hinted. It's, well, a first test of the hinting.

@BoboTiG
Copy link
Collaborator

BoboTiG commented Mar 14, 2023

Fully agree 👍🏻

@altendky altendky marked this pull request as ready for review March 14, 2023 22:00
@altendky altendky mentioned this pull request Mar 14, 2023
3 tasks
@BoboTiG
Copy link
Collaborator

BoboTiG commented Mar 14, 2023

It seems there are regressions in macOS tests.

FTR:

  • tests/test_0_watchmedo.py::test_auto_restart_subprocess_termination[False] is known to be a failure on Windows, and sometimes (quite often) on macOS too.
  • tests/test_0_watchmedo.py::test_auto_restart_on_file_change_debounce is a frequent failure on all Oses

@BoboTiG
Copy link
Collaborator

BoboTiG commented Mar 14, 2023

I think we could just skip pypy tests on Windows, they never make it (enven if it's temporary, maybe someone later will find some time/motivation to dig into that problem).

And now that I see soo many PRs (💪🏻) the release job being run every time may not be a good idea finally, WDYT?

@altendky
Copy link
Contributor Author

I think we could just skip pypy tests on Windows, they never make it.

I guess we could also document testing status and plans in a dedicated issue instead of email as I had initiated. Either way.

@altendky
Copy link
Contributor Author

And now that I see soo many PRs (💪🏻) the release job being run every time may not be a good idea finally, WDYT?

Personally, I wouldn't. I don't want to find that broken when I'm trying to make a release. But, I haven't really looked into that one to see what's going on there.

@BoboTiG
Copy link
Collaborator

BoboTiG commented Mar 14, 2023

I'll open issues for the tests 👍🏻

@altendky
Copy link
Contributor Author

To be clear, I meant that for general test work. Regressions here I'll just look at here.

@BoboTiG
Copy link
Collaborator

BoboTiG commented Mar 14, 2023

Yup, I got it :)

@altendky
Copy link
Contributor Author

I do expect to put some time into the state of the tests, but figured I'd wait until I heard something about the status. Even if that is 'they are having trouble and someone needs to do the research as to what they need because we don't know yet'.

@BoboTiG
Copy link
Collaborator

BoboTiG commented Mar 15, 2023

Ready to merge on my end.

@altendky
Copy link
Contributor Author

Should I add more xfails or flake retries for these tests? But, I'm not aware of anything else to be done here.

@BoboTiG
Copy link
Collaborator

BoboTiG commented Mar 15, 2023

It's OK then. Those tests are "pure randoms" if I may.

@BoboTiG BoboTiG merged commit 764a234 into gorakhargosh:master Mar 15, 2023
@BoboTiG
Copy link
Collaborator

BoboTiG commented Mar 15, 2023

That's an impressive job, thanks a lot @altendky 🍾

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