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.

Sign up for GitHub

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

Jump to bottom

Deprecate the --partial options #108

Closed
mtreinish opened this issue Oct 5, 2017 · 4 comments
Closed

Deprecate the --partial options #108

mtreinish opened this issue Oct 5, 2017 · 4 comments
Labels
enhancement help wanted

Comments

@mtreinish
Copy link
Owner

mtreinish commented Oct 5, 2017 •
edited
Loading

Tracing through the code the use of --partial on stestr run and stestr load don't seem to have any real effect. This option was directly ported from testr in the initial fork and I didn't really think about it but the flags serve no real purpose from what I can tell. I think the original idea was to distinguish an entry in a repository for a full run vs one that was a subset. (but you'd have to check with the testrepository authors on that) But nothing else respects or expects that difference, and from an end user perspective it doesn't really matter. The repository contains the historical record of previous runs and information for scheduling is aggregated independently from that. So it doesn't really matter if the record is for a partial execution or not.

Therefore I think it'll be good to just remove those options, Unfortunately the options are in our stable interfaces (both the cli and python commands api) so we'll have to deprecate them for a bit first. We can remove the flags and just print a deprecation warning on usage to warn people things are going away.

The only functional code that seems to use it is in the repository:

stestr/stestr/repository/file.py

Lines 314 to 322 in 86b8ee4

if self.partial:
# Seed with current failing
inserter = testtools.ExtendedToStreamDecorator(repo.get_inserter())
inserter.startTestRun()
failing = self._repository.get_failing()
failing.get_test().run(inserter)
inserter.stopTestRun()
inserter = testtools.ExtendedToStreamDecorator(
repo.get_inserter(partial=True))

and

stestr/stestr/repository/memory.py

Lines 163 to 164 in 82b3efc

if not self._partial:
self._repository._failing = OrderedDict()

Which are related, the file repo uses the memory repo call after that highlighted if statement. I'm not exactly sure on the function of that code so investigation will be needed to understand what's being triggered there and how to implement it without a user facing flag.
@mtreinish mtreinish mentioned this issue Oct 17, 2017
Merged
mtreinish added a commit that referenced this issue Oct 17, 2017
@mtreinish
Deprecate the partial flag
11de57e 
The partial flag is vestigial and left over from the original testr fork.
It was originally used to indicate a run did not execute the entire test
suite. However this doesn't really come into play anywhere else and
it's not stored in the repository either. I think the eventual intent
was to treat those records as incomplete for other purposes but that
never really was implemented. With the introduction of load to run-id
and also a run --combine this isn't really needed anymore either. It's not
really something that should be user facing either. The user shouldn't
have to tell the runner whether the run is the full suite or not.

This commit removes the functional bits from the cli (and their python
api commands) and deprecates all the flags for partial. The repository
partial usage remains intact because it's coupled to how the file
repository runs. (it uses a memory repository with partial=True before
writing it to disk) So more care is needed before we rip that out, but
in the mean time this removes and deprecates all the end user facing
pieces.

Closes issue #108
mtreinish added a commit that referenced this issue Oct 18, 2017
@mtreinish
Deprecate the partial flag
b531108 
The partial flag is vestigial and left over from the original testr fork.
It was originally used to indicate a run did not execute the entire test
suite. However this doesn't really come into play anywhere else and
it's not stored in the repository either. I think the eventual intent
was to treat those records as incomplete for other purposes but that
never really was implemented. With the introduction of load to run-id
and also a run --combine this isn't really needed anymore either. It's not
really something that should be user facing either. The user shouldn't
have to tell the runner whether the run is the full suite or not.

This commit removes the functional bits from the cli (and their python
api commands) and deprecates all the flags for partial. The repository
partial usage remains intact because it's coupled to how the file
repository runs. (it uses a memory repository with partial=True before
writing it to disk) So more care is needed before we rip that out, but
in the mean time this removes and deprecates all the end user facing
pieces.

Closes issue #108
mtreinish added a commit that referenced this issue Oct 22, 2017
@mtreinish
Deprecate the partial flag
aa2059c 
The partial flag is vestigial and left over from the original testr fork.
It was originally used to indicate a run did not execute the entire test
suite. However this doesn't really come into play anywhere else and
it's not stored in the repository either. I think the eventual intent
was to treat those records as incomplete for other purposes but that
never really was implemented. With the introduction of load to run-id
and also a run --combine this isn't really needed anymore either. It's not
really something that should be user facing either. The user shouldn't
have to tell the runner whether the run is the full suite or not.

This commit removes the functional bits from the cli (and their python
api commands) and deprecates all the flags for partial. The repository
partial usage remains intact because it's coupled to how the file
repository runs. (it uses a memory repository with partial=True before
writing it to disk) So more care is needed before we rip that out, but
in the mean time this removes and deprecates all the end user facing
pieces.

Closes issue #108
mtreinish added a commit that referenced this issue Oct 25, 2017
@mtreinish
Deprecate the partial flag
dd11975 
The partial flag is vestigial and left over from the original testr fork.
It was originally used to indicate a run did not execute the entire test
suite. However this doesn't really come into play anywhere else and
it's not stored in the repository either. I think the eventual intent
was to treat those records as incomplete for other purposes but that
never really was implemented. With the introduction of load to run-id
and also a run --combine this isn't really needed anymore either. It's not
really something that should be user facing either. The user shouldn't
have to tell the runner whether the run is the full suite or not.

This commit removes the functional bits from the cli (and their python
api commands) and deprecates all the flags for partial. The repository
partial usage remains intact because it's coupled to how the file
repository runs. (it uses a memory repository with partial=True before
writing it to disk) So more care is needed before we rip that out, but
in the mean time this removes and deprecates all the end user facing
pieces.

Closes issue #108
@masayukig
Copy link
Collaborator

oh, github doesn't support the "Partial-Bug" keyword :-p

@mtreinish
Copy link
Owner Author

heh, I don't think github supports any keywords except for the '#' references. I haven't found any automation around closing issues. Although I haven't bothered to read any of the documentation on it.

@mtreinish
Copy link
Owner Author

@masayukig do you think we can close this since we landed the deprecation patch? Or do you think we should wait until the removal patch at some point in the future (it'll probably be a long time because it's an annoying breaking change)

@masayukig
Copy link
Collaborator

Yeah, I prefer closing this issue and make a new issue for removing the --partial option. And it's better to change the title of this issue to like "Deprecate the --partial options".

@mtreinish mtreinish changed the title Deprecate and remove the --partial options Deprecate the --partial options Nov 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement help wanted
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@masayukig @mtreinish