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

Allow tests to run on MacOS. #2092

Merged
merged 1 commit into from Jul 17, 2023
Merged

Allow tests to run on MacOS. #2092

merged 1 commit into from Jul 17, 2023

Conversation

janiversen
Copy link
Contributor

MacOS is a little bit different than Linux basically it puts "/private" in front of a number of standard paths.

I added conftest.py for now just with 2 tests IS_DARWIN and IS_WINDOWS, but I believe there are more common items between the test files, that could later go into conftest.py

I changed a few test in a way that the difference in path is very clear, which I prefer because "you see what you get" :-) but if there is a preference for a central function I can easily make that.

I am (currently) not trying to make mopidy work on MacOS, that would take a lot more testing, I just want a clean environment so that I can test locally before submitting my bug fixes.

@janiversen
Copy link
Contributor Author

I knew it, I am so used to having a precommit hook, that I forgot to run lint, let me just do that.

@kingosticks
Copy link
Member

kingosticks commented Jul 14, 2023

I'd personally prefer to not think about what platform I am running on when writing tests, it's just unrelated noise. I'd much rather hide this in a helper.

However, I am a bit surprised we also we need this. I don't have an MacOS system but isn't /tmp normally a symlink to /private/tmp so it'll just work in most cases. For those that don't, can we just .resolve() the path first?

Edit: clarity

@kingosticks kingosticks added the C-enhancement Category: A PR with an enhancement or an issue with an enhancement proposal label Jul 14, 2023
@janiversen
Copy link
Contributor Author

Just to be clear we talk about MacOS not IOS!

Getting pytest and not to mention python itself, is a major challenge.

/tmp and /private/tmp are not symlinked on Mac, when using pathlib which tries to give the "real" path.

Let me see if I can do magic with .resolve() or alternatively I can make a helper making it (nearly) invisible.

Btw. I agree with you, I too hate having platform specific items in test, in my other project we have managed to reduce it to just a couple of tests.

@janiversen
Copy link
Contributor Author

Forgot one comment. I have only changed where needed (I live by the rule "never touch a running system"), however in most cases it was less changed to do it generally in the setup().

@janiversen
Copy link
Contributor Author

Found the culprit !!!

It is actually .resolve() that follows symlinks and return the physical path.

Please see def expand_path(path) in path.py, I added a couple of tmps to debug better:

    # original statement:
    return pathlib.Path(path).expanduser().resolve()

    # my additions:
    path = '/var/folders/r_/ln...gn/T/tmpkqzcbhdb/test'
    tmp1 = pathlib.Path(path) 
    tmp2 = tmp1.expanduser()
    tmp3 = tmp2.resolve() # tmp3 == PosixPath(

    # tmp1 == PosixPath('/var/folders/r_/ln...gn/T/tmpkqzcbhdb/test')
    # tmp2 == PosixPath('/var/folders/r_/ln...gn/T/tmpkqzcbhdb/test')
    # tmp3 == PosixPath('/private/var/folders/r_/ln...gn/T/tmpkqzcbhdb/test')

Now that I know what the problem is, let me see how I can fix it.

I do not want to change the production code, only the test code.

@janiversen
Copy link
Contributor Author

@kingosticks Thanks for the feedback

I like positive review comments...they make you think and not just do what you usually do.

I have reworked the patch and removed all the platform dependent pars.

I hope you like it, or tell me what I would change. It is a little bit strange to call .resolve(), I think it should be moved down in the production code, but at a much later date.

Your review comments gave me the opportunity to start debugging the core, I will continue with that for my next PR (slowly learning more about the code and the project).

@janiversen
Copy link
Contributor Author

@kingosticks forgot to mention, it is ready for another review and hopefully a merge, when you have time

@kingosticks kingosticks added this to the Next bug fix release milestone Jul 17, 2023
@kingosticks kingosticks merged commit fcf1156 into mopidy:develop Jul 17, 2023
11 checks passed
@janiversen janiversen deleted the tests branch July 17, 2023 14:41
@jodal jodal modified the milestones: Next bug fix release, v4.0.0 Oct 29, 2023
jodal pushed a commit that referenced this pull request Oct 31, 2023
@jodal jodal modified the milestones: v4.0.0, v3.4.2 Oct 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: A PR with an enhancement or an issue with an enhancement proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants