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

Move MockOptions into testutil library #9086

Closed
wants to merge 1 commit into from

Conversation

gshuflin
Copy link
Contributor

@gshuflin gshuflin commented Feb 7, 2020

The test for the v2 test goal defines a MockOptions type since it specifically needs to create an options mock. But this is really a more general piece of functionality that ought to live in a testutil library so other test files can make use of it.

Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

I'm not convinced that this is how we want to mock a GoalSubsystem. For example, we might want to leverage the prior art of init_subsystem. I think we should wait to see if we can come up with anything better.

Copy link
Contributor

@cosmicexplorer cosmicexplorer left a comment

Choose a reason for hiding this comment

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

Great thinking!

@@ -170,6 +171,13 @@ def remove_locations_from_traceback(trace):
return new_trace


class MockOptions:
"""Test mock for any options type."""
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"""Test mock for any options type."""
"""Test mock for any `Optionable` subclass."""

@cosmicexplorer
Copy link
Contributor

For example, we might want to leverage the prior art of init_subsystem.

I agree with this in theory, but it has also been super annoying to correctly use init_subsystem() for me personally in tests, so this approach seems quite nice to me. Perhaps a halfway point could be to validate that the options registered in MockOptions have been correctly registered?

@Eric-Arellano
Copy link
Contributor

Ideally, a solution will fix this:

@pytest.mark.skip(
reason="Figure out how to create a GoalSubsystem for tests. We can't call global_instance()"
)
def test_line_oriented_goal() -> None:
class OutputtingGoalOptions(LineOriented, GoalSubsystem):
name = "dummy"
class OutputtingGoal(Goal):
subsystem_cls = OutputtingGoalOptions
@goal_rule
def output_rule(console: Console, options: OutputtingGoalOptions) -> OutputtingGoal:
with options.output(console) as write_stdout:
write_stdout("output...")
with options.line_oriented(console) as print_stdout:
print_stdout("line oriented")
return OutputtingGoal(0)
mock_console = MockConsole()
# TODO: how should we mock `GoalSubsystem`s passed to `run_rule`?
mock_options = Mock()
mock_options.output = OutputtingGoalOptions.output
mock_options.line_oriented = OutputtingGoalOptions.line_oriented
result: OutputtingGoal = run_rule(output_rule, rule_args=[mock_console, mock_options])
assert result.exit_code == 0
assert mock_console.stdout.getvalue() == "output...line oriented"

If we use Mock, there, then we end up not even testing that GoalSubsystem works properly with Outputting becuase the Mock object won't be a real GoalSubsystem.

I won't block on this, and do appreciate trying to make testing a better experience, only don't want to put something in a public API until we explored other possibilities.

@cosmicexplorer
Copy link
Contributor

I think the above argument is reasonable enough. Is it clear how to rewrite the tests in test_test.py in a way that uses a real GoalSubsystem, or would there need to be some separate infrastructure required to do that?

@Eric-Arellano
Copy link
Contributor

Superseded by John creating a proper utility in #9528.

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.

3 participants