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

fix tests on macOS #81

Closed
wants to merge 4 commits into from

Conversation

jeremyschlatter
Copy link

Hello! I noticed that some of the tests are failing on macOS. I implemented a small fix. Happy to take feedback on it.

Details below.


The tests assume that the config dir is ~/.config and the data dir is ~/.local/share. Neither is true on macOS, and this causes several tests to fail:

FAILED tests/test_emborg.py::test_emborg_without_configs[4] - AssertionError: Test ‘labor’ fails.
FAILED tests/test_emborg.py::test_emborg_with_configs[5] - AssertionError: Test ‘periphery’ fails.
FAILED tests/test_emborg.py::test_emborg_with_configs[25] - AssertionError: Test ‘build’ fails.
FAILED tests/test_emborg.py::test_emborg_overdue[1] - AssertionError: Test ‘predator’ fails.
FAILED tests/test_emborg.py::test_emborg_overdue[2] - AssertionError: Test ‘smelt’ fails.

Rather than changing the tests to dynamically use different test paths based on the OS[1], it's simpler to change emborg's behavior at test time to always use the Linux paths. That is what this change does.

Also adds tests/Users to .gitignore, which is the macOS equivalent of tests/home.

[1] This was in fact the first thing I tried. But as I got into it the added complexity didn't seem justified.

@coveralls
Copy link

coveralls commented Aug 15, 2024

Pull Request Test Coverage Report for Build 10426428059

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 63.94%

Totals Coverage Status
Change from base Build 10407960020: 0.0%
Covered Lines: 1970
Relevant Lines: 2887

💛 - Coveralls

@KenKundert
Copy link
Owner

I'm not crazy about modifying the emborg executable to support a special testing mode.

Is it possible to resolve this issue simply by adding some symbol links in the tests directory so that the paths needed by the mac exist and point to the ones provided for linux?

I'm afraid I do not have a Mac and have been unable to get testing with a Mac working in github actions, so it is difficult for me to resolve this myself.

The tests assume that the config dir is ~/.config and the data dir is
~/.local/share. Neither is true on macOS, and this causes several tests
to fail:

	FAILED tests/test_emborg.py::test_emborg_without_configs[4] - AssertionError: Test ‘labor’ fails.
	FAILED tests/test_emborg.py::test_emborg_with_configs[5] - AssertionError: Test ‘periphery’ fails.
	FAILED tests/test_emborg.py::test_emborg_with_configs[25] - AssertionError: Test ‘build’ fails.
	FAILED tests/test_emborg.py::test_emborg_overdue[1] - AssertionError: Test ‘predator’ fails.
	FAILED tests/test_emborg.py::test_emborg_overdue[2] - AssertionError: Test ‘smelt’ fails.

Rather than changing the tests to dynamically use different test paths
based on the OS[1], it's simpler to change emborg's behavior at test
time to always use the Linux paths. That is what this change does.

Also adds tests/Users to .gitignore, which is the macOS equivalent of
tests/home.

[1] This was in fact the first thing I tried. But as I got into it the
    added complexity didn't seem justified.
@jeremyschlatter
Copy link
Author

jeremyschlatter commented Aug 16, 2024

Makes sense that you're reluctant to add test-only code to the executable.

Adding symbolic links is a nice idea, but unfortunately doesn't solve the problem.

It is possible to solve the problem by modifying only the tests and not the executable. I've pushed a new version that does that. As you can see, it's a significantly bigger change, but might be better depending on your taste. I'm interested to see what you think.

@KenKundert
Copy link
Owner

Yes, it is a much larger change. And I am starting to rethink your original version. As I said, I am not crazy about modifying the executable to support a testing mode, but I have previously heard from a Mac user that they use the Linux config file conventions on the Mac because they find the Mac conventions awkward. So if we change the EMBORG_TEST environment variable to EMBORG_CONFIG_DIR and EMBORG_LOG_DIR it turns the whole thing into a feature that allows users to choose the directories. Give me a couple of days to think about this. I am distracted at the moment but I will try to resolve this within a week or two.

And thank you for providing both implementations. I am sorry for putting you through the trouble of creating the second version.

@KenKundert
Copy link
Owner

Actually, there may be another related solution. Emborg uses the appdirs module to find the config and data directories. According to the appdirs documentation, it appears to use XDG, which suggests that one can control the location of the config and data dir using XDG_CONFIG_HOME and XDG_DATA_HOME environment variables. Unfortunately, on MacOS appdirs ignores XDG and uses hardcoded paths unique to the Mac. Rather than using EMBORG_CONFIG_DIR and EMBORG_LOG_DIR emborg could honor the XDG variables if defined and ignore the ones provided by appdirs.

I am not sure that this is better than going with emborg specific environment variables, but it is an alternative I am considering.

@KenKundert
Copy link
Owner

Thinking more about this, presumably the tests will not work on Windows machines any more than they work on Macs. And using your first solution will presumably resolve the issues on Windows while your second solution will not. That is another reason to go with the enviornment variables approach.

@KenKundert
Copy link
Owner

Here is another alternative. emborg could use the directory suggested by appdirs if it exists, otherwise is defaults to the XDG or Linux conventions.

@KenKundert
Copy link
Owner

In digging in to this I found that emborg was already configured to use ~/.config/avendesora if it existed, even on a Mac. This was due to an earlier request from a Mac user that did not care for the default location of configuration directories on the Mac. So presumably the problem you were having was not due to the config directory. Perhaps there is something else that is the issue.

@KenKundert
Copy link
Owner

I have modified emborg to support the XDG_CONFIG_HOME and XDG_DATA_HOME environment variables. If these environment variables are set, emborg will use their values as the location of the shared config and data directories. I set these values before starting the tests so the tests should always use the pre-configured config directory in the tests directory.

Please give it a try and see if it resolves your issue. The version of github is the one that contains the changes.

@jeremyschlatter
Copy link
Author

That fixed it. Thanks, @KenKundert!

@jeremyschlatter jeremyschlatter deleted the fix-macos branch August 29, 2024 20:08
@jeremyschlatter
Copy link
Author

Oh, and I just saw your previous comment:

In digging in to this I found that emborg was already configured to use ~/.config/avendesora if it existed, even on a Mac. This was due to an earlier request from a Mac user that did not care for the default location of configuration directories on the Mac. So presumably the problem you were having was not due to the config directory. Perhaps there is something else that is the issue.

I had noticed this too. But the test setup does not automatically create this directory at first, so that logic does not trigger. I considered creating that directory in the test setup, but the test labor expects it not to exist, and checks that emborg creates it.

The XDG solution works well. Just wanted to add that note to the record 🙂

@KenKundert
Copy link
Owner

Thanks Jeremy

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.

3 participants