-
Notifications
You must be signed in to change notification settings - Fork 3
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
Migrate or remove old repository unit tests #296
Conversation
…e to changes to system design.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all looks sensible to me. Since the rest of the change is making a point of adding type hinting, there are a couple of places I think more could be added.
assert process.returncode == -15 | ||
|
||
@mark.skip(reason="Too hard to test at the moment.") | ||
def test_extract_from_http(self, workspace, tmp_path: Path): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should workspace have a type hint here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
|
||
@mark.skipif(not find_executable('git-daemon'), | ||
reason="Unable to find git daemon") | ||
def test_extract_from_git(self, workspace, tmp_path: Path): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should workspace have a type hint here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also yes.
from fab.build_config import BuildConfig | ||
from fab.steps.grab.svn import split_repo_url, svn_export | ||
|
||
from .support import Workspace, file_tree_compare | ||
|
||
|
||
class TestRevision(object): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
object argument is obsolete in python3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
from pathlib import Path | ||
from subprocess import Popen, run | ||
import time | ||
from typing import Tuple |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is missing a few type hints from new methods. Specifically for TempPathFactory (which would need an additional import) and Workspace
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
This chnage has become totally unwieldy with all the style fixes. I'm shuttting it down so the repository migration can happen and I'll start again from scratch with separate changes. |
Salvage some value from the technology demonstrator implementation, unit test wise. Also some minor type hinting, documentation and trying to work out what any of this stuff does. This change only covers repository access unit tests.