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

Implement proof of concept formatter tests #426

Merged
merged 5 commits into from
Jun 4, 2021
Merged

Implement proof of concept formatter tests #426

merged 5 commits into from
Jun 4, 2021

Conversation

splondike
Copy link
Collaborator

@splondike splondike commented May 8, 2021

This pull request implements a proof of concept for running unittests outside of the Talon environment (e.g. via github actions) while making sure we don't disturb things when running within Talon. You can see I've got a single test case on one of the formatters running and passing.

Some things to consider:

  1. Is the built in unittest module fine or do we want to use pytest?
  2. Are we ever likely to want to run tests under the actual Talon runtime? I'm not sure how to do that immediately, but I think it'd be some weird REPL thing currently. If we might want that, then we might consider how to do that a little bit here to make sure we're not going to make that difficult later.
  3. Does this run fine on Windows and Mac?

If everybody thinks this looks like a good addition I'll add something to the README and merge it in. The next step down this journey (aside from writing more tests) would be setting up #381 and integrating with that.

@splondike splondike marked this pull request as draft May 8, 2021 10:15
@dwiel
Copy link
Collaborator

dwiel commented May 8, 2021 via email

@knausj85
Copy link
Member

This looks like a great start.

  1. I'd probably vote for pytest
  2. At the moment, I don't see any particular need to run things from within Talon. Did you have a use case in mind?
    Long term, though, the path to do so would probably be via some help from aegis (https://talonvoice.slack.com/archives/C9MHQ4AGP/p1618510915484700?thread_ts=1618392434.412400&cid=C9MHQ4AGP) depending on the desires.
  3. Yes, it works fine on Windows and Mac.

@splondike
Copy link
Collaborator Author

OK, I'll give pytest a try on the weekend.

Yeah, I don't have a compelling case for testing within the Talon runtime currently, I won't worry about it for the moment. I don't expect we'll have too many tests in this repo anyway.

@splondike
Copy link
Collaborator Author

Okay so I have swapped it over to using pytest. You can run it using pytest.

There was a bit of a complication setting it up because pytest uses the built in Python code module (unlike unittest). This means that by the time my test code is running and wants to import from the knausj code module the other one is already registered. I worked around this using a symlink. See the conftest.py file for a bit of an explanation.

If this looks good to y'all now I'll add something to the README about how to run it and then merge it.

@splondike splondike marked this pull request as ready for review May 16, 2021 05:17
@splondike splondike requested a review from lunixbochs May 29, 2021 10:30
@@ -0,0 +1,3 @@
This directory contains stubs of Talon APIs. It is included via run_tests.py so we can run our test cases outside the Talon environment.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Faulty comment.

conftest.py Outdated
# look at customising the python module loader:
# https://docs.python.org/3/reference/import.html
# I've had a go at relative imports without success.
symlink_src = os.path.join(curr_dir, "code")
Copy link
Collaborator Author

@splondike splondike May 29, 2021

Choose a reason for hiding this comment

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

From aegis:

just need a PathFinder in sys.meta_path I think
New
8:39
looks something like this:

class PluginFinder(PathFinder):
    @classmethod
    def find_spec(cls, fullname, path=None, target=None):
        return super().find_spec(fullname, path, target)
sys.meta_path.insert(0, PluginFinder)

(edited)
8:40
if you're passed a None path, you need to use the fullname to figure out the file path. otherwise, check if the path makes sense for a path you are willing to load from (edited) 
8:40
and find_spec can return None if you couldn't find anything
perhaps:

- from knausj_code import formatters
+ from user.knausj_talon.code import formatters

8:42
so then you take any fullname starting with user.knausj_talon. and rebase it to the root of your repo

you'd probably also handle from talon import ... from the PluginFinder

@splondike
Copy link
Collaborator Author

@lunixbochs Re-implemented the pytest import manipulations as an importlib loader. It's a little bit clunky with the root module loader, but seems to work OK. Ready for re-review.

@lunixbochs
Copy link
Collaborator

should the draft window tests use pytest too?

@splondike
Copy link
Collaborator Author

Yeah, probably should though pytest happily runs the unittest format as well. Sounds like nobody has objections to the shape of this PR so I'll do that README modification and merge it in on the weekend. Will probably swap the draft window tests to pytest also then.

After that I'm going to look into the formatters a bit more, I'm not sure they do the right thing with symbols. And after that maybe Github Actions so we can have a CI.

@splondike splondike merged commit bfecb77 into talonhub:master Jun 4, 2021
@splondike splondike deleted the feature/unittests branch June 4, 2021 07:33
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.

4 participants