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

Improve documentation and usage of signac.testing #709

Closed
vyasr opened this issue Mar 5, 2022 · 7 comments · Fixed by #863
Closed

Improve documentation and usage of signac.testing #709

vyasr opened this issue Mar 5, 2022 · 7 comments · Fixed by #863
Labels
documentation Writing or editing documentation

Comments

@vyasr
Copy link
Contributor

vyasr commented Mar 5, 2022

Feature description

It would be helpful to have some standard methods for populating a signac Project, both for signac's own tests and for developer prototyping. Currently signac.testing contains a helpful function for creating jobs in a project, but this functionality is largely untested, it is unused even in our own tests, and there's no visible documentation for it.

Proposed solution

We should improve the documentation and visibility of the existing functionality of signac.testing, and perhaps we should flesh it out in order to support broader usage based on the needs of our existing tests.

Additional context

https://github.com/glotzerlab/signac/pull/685/files/5f61c44ce0ac653633baaa6e1d20138e9ccc899b#r813880252

@stale
Copy link

stale bot commented Jun 12, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jun 12, 2022
@stale stale bot closed this as completed Jul 31, 2022
@cbkerr cbkerr added the documentation Writing or editing documentation label Aug 15, 2022
@vyasr
Copy link
Contributor Author

vyasr commented Nov 6, 2022

Let's make a decision on what to do with this. If we want to leverage signac.testing in tests then we should augment this module with whatever it needs for that use case.

@vyasr vyasr reopened this Nov 6, 2022
@stale stale bot removed the stale label Nov 6, 2022
@bdice
Copy link
Member

bdice commented Nov 6, 2022

While I agree with the premise of this issue that standard methods for initializing test data could be helpful, I'd vote to remove signac.testing. I have never used it. It's only used once in signac tests.

jobs = signac.testing.init_jobs(

Moreover, it's supporting us keeping unnecessary baggage in signac-flow. The function signac.testing.init_jobs is used in signac-flow for a similarly untested, also unused flow.testing.make_project function.
https://github.com/glotzerlab/signac-flow/blob/4d49e4f72e8d2201fa27efe2fbb3b0940a31ca79/flow/testing.py#L10

And a corresponding project template (we really don't need project templates in flow...):
https://github.com/glotzerlab/signac-flow/blob/4d49e4f72e8d2201fa27efe2fbb3b0940a31ca79/flow/templates/project_testing.pyt#L49

@vyasr
Copy link
Contributor Author

vyasr commented Nov 7, 2022

That was basically my original position: I agree that this is a nice idea, but in practice none of us use it so it might not be the best starting point. @csadorf do you still think this is worth doing? An alternative would be to remove this for 2.0, but the next time we do any refactoring of tests we make an effort to extract this type of logic from tests into a single reusable function/fixture that we can expose publicly afterwards.

@csadorf
Copy link
Contributor

csadorf commented Nov 7, 2022

I am a bit surprised that this is not used in practice, considering that similar components exist in many other frameworks, but if that's the reality, I see no reason to keep it.

@bdice
Copy link
Member

bdice commented Nov 7, 2022

If we are tired of making changes for 2.0, I have no problem with keeping it and leaving the status quo. This can be cleaned up and expanded OR removed in a future release. I don’t think the change is essential for 2.0.

@bdice
Copy link
Member

bdice commented Nov 16, 2022

Discussed today at the developer meeting: the signac.testing module can be removed. We can also remove its use in flow.testing.make_project. We agreed this could be done without deprecation in flow, since the feature is not used or tested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Writing or editing documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants