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

Migrate or remove old repository unit tests #296

Closed
wants to merge 28 commits into from
Closed

Migrate or remove old repository unit tests #296

wants to merge 28 commits into from

Conversation

MatthewHambley
Copy link
Collaborator

@MatthewHambley MatthewHambley commented Mar 18, 2024

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.

@MatthewHambley MatthewHambley changed the title Migrate or remove old unit tests Migrate or remove old repository unit tests Apr 4, 2024
Copy link
Collaborator

@ericaneininger ericaneininger left a 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):
Copy link
Collaborator

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?

Copy link
Collaborator Author

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):
Copy link
Collaborator

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?

Copy link
Collaborator Author

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):
Copy link
Collaborator

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

Copy link
Collaborator Author

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
Copy link
Collaborator

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

@MatthewHambley
Copy link
Collaborator Author

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.

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