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

Fixup ProcessLauncherTests issues #746

Merged
merged 5 commits into from
Aug 20, 2019
Merged

Fixup ProcessLauncherTests issues #746

merged 5 commits into from
Aug 20, 2019

Conversation

drawlerr
Copy link
Contributor

@drawlerr drawlerr commented Aug 16, 2019 •
edited
Loading

The start_stop test in ProcessLauncherTests uses a MagicMock object for Process, which is incompatible with telemetry operations.

  • Replace Process MagicMock with a minimal hand-mocked class.
  • Also, remove Config + MetricsStore mocks while we're at it.

Relates to PRs #743, #744

@drawlerr drawlerr changed the title Cleanup ProcessLauncherTests Fixup ProcessLauncherTests issues Aug 16, 2019
@drawlerr drawlerr added :misc Changes that don't affect users directly: linter fixes, test improvements, etc. :Telemetry Telemetry Devices that gather additional metrics enhancement Improves the status quo labels Aug 16, 2019
@drawlerr drawlerr added this to the 1.3.0 milestone Aug 16, 2019
This was referenced Aug 16, 2019
Closed
Merged
@drawlerr drawlerr removed the enhancement Improves the status quo label Aug 16, 2019
@drawlerr drawlerr requested a review from ebadyano August 19, 2019 14:19
danielmitterdorfer
danielmitterdorfer reviewed Aug 20, 2019
View reviewed changes
Copy link
Member

@danielmitterdorfer danielmitterdorfer left a comment

Choose a reason for hiding this comment

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

Thank you for this improvement. It looks mostly fine to me but I left a few comments.

tests/mechanic/launcher_test.py Show resolved Hide resolved
tests/mechanic/launcher_test.py Outdated Show resolved Hide resolved
tests/mechanic/launcher_test.py Outdated Show resolved Hide resolved
@drawlerr drawlerr merged commit 2835254 into elastic:master Aug 20, 2019
@drawlerr drawlerr deleted the cleanup-plt branch August 20, 2019 18:57
@danielmitterdorfer danielmitterdorfer added the enhancement Improves the status quo label Sep 9, 2019
novosibman pushed a commit to novosibman/rally that referenced this pull request Oct 2, 2019
@drawlerr @novosibman
Fixup ProcessLauncherTests issues (elastic#746)
3b6a942
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danielmitterdorfer danielmitterdorfer danielmitterdorfer left review comments

@dliappis dliappis Awaiting requested review from dliappis

@ebadyano ebadyano Awaiting requested review from ebadyano

Assignees
No one assigned
Labels
enhancement Improves the status quo :misc Changes that don't affect users directly: linter fixes, test improvements, etc. :Telemetry Telemetry Devices that gather additional metrics
Projects
None yet
Milestone
1.3.0
Development

Successfully merging this pull request may close these issues.
2 participants
@drawlerr @danielmitterdorfer