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

feat: [SYNC-3835] pydocstyle implementation #585

Merged
merged 13 commits into from
Feb 5, 2024

Conversation

taddes
Copy link
Contributor

@taddes taddes commented Jan 30, 2024

References

JIRA: SYNC-3835

Description

Addition of pydocstyle to python testing section of autopush.

Acceptance Criteria

  • pydocstyle is added and applied as a dev dependency for the Python test solutions
    
  • Configurations can be set in the pyproject.toml
    
  • The CI is updated to execute pydocstyle
    
  • Documentation is updated accordingly
    

The following has been implemented:

  • Added pydocstyle to the lint command and also implemented a make pydocstyle command that can be called on its own for a richer output on recommendations.
  • Added pydocstyle to run as part of CI flow.
  • Added .install.stamp to manage poetry dependency installation whenever commands invoked. This was then added to .gitignore and .dockerignore
  • Integration tests have docstring annotations describing the test functions. Note that this was done rather simply, mimicking the name of the test function. Given we will modernize and refactor the integration tests, it didn't seem prudent to waste too much time on this.
  • Load test docstrings updated. Since it seemed the those were hedging towards the NumPy standard of docstrings, I made the decision to follow that convention over reStructuredText, since it is a bit more readable. Both are official implementations supported by Sphinx. If someone prefers reStructuredText, that can be used instead.

PR Review Checklist

Put an x in the boxes that apply

  • This PR conforms to the Contribution Guidelines
  • The PR title starts with the JIRA issue reference, format [SYNC-####], and has the same title (if applicable)
  • Documentation has been updated (if applicable
  • Functional and performance test coverage has been expanded and maintained (if applicable)

@taddes taddes self-assigned this Jan 30, 2024
@taddes taddes force-pushed the SYNC-3835-pydocstyle-implementation branch from 477fb7d to 1dc9ed8 Compare January 30, 2024 15:40
Copy link
Member

@jrconlin jrconlin left a comment

Choose a reason for hiding this comment

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

Added a few more descriptive titles for things. That way the titles aren't just "title" spaceholders.

tests/integration/test_integration_all_rust.py Outdated Show resolved Hide resolved
tests/integration/test_integration_all_rust.py Outdated Show resolved Hide resolved
tests/integration/test_integration_all_rust.py Outdated Show resolved Hide resolved
tests/integration/test_integration_all_rust.py Outdated Show resolved Hide resolved
tests/integration/test_integration_all_rust.py Outdated Show resolved Hide resolved
tests/integration/test_integration_all_rust.py Outdated Show resolved Hide resolved
tests/integration/test_integration_all_rust.py Outdated Show resolved Hide resolved
tests/integration/test_integration_all_rust.py Outdated Show resolved Hide resolved
tests/integration/test_integration_all_rust.py Outdated Show resolved Hide resolved
tests/integration/test_integration_all_rust.py Outdated Show resolved Hide resolved
@taddes taddes force-pushed the SYNC-3835-pydocstyle-implementation branch from f775454 to 5aed9e3 Compare January 30, 2024 23:09
@taddes taddes changed the title WIP: [SYNC-3835] pydocstyle implementation feat: [SYNC-3835] pydocstyle implementation Jan 30, 2024
@taddes taddes requested a review from jrconlin January 30, 2024 23:26
@taddes taddes marked this pull request as ready for review January 31, 2024 03:42
-------
TickTuple | None

Distribution Parameters
Copy link
Contributor

Choose a reason for hiding this comment

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

I've not heard of the term 'Distribution Parameters' before. For clarity user_count spawn_rate and user_classes are the fields in the return type TickTuple, having them defined at the same indentation as None, the alternative return type to TickTuple I think may be confusing in future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will take it out, even though it was there before??? Let me know if the change to just enumerate the contained parameters is clearer.

    def tick(self) -> TickTuple | None:
        """Override defining the desired distribution for Autopush load testing.

        Returns:
            TickTuple | None 

            TickTuple Contained Parameters
                user_count: Total user count
                spawn_rate: Number of users to start/stop per second when changing
                            number of users
                user_classes: None or a List of user classes to be spawned
                
            None: Instruction to stop the load test
        """

return self.environment.autopush_wait_time(self)

def on_start(self) -> Any:
"""Called when a User starts running."""
"""Call when a websocket starts running."""
Copy link
Contributor

Choose a reason for hiding this comment

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

Is 'Callback' maybe a better word for the nature of these lifecycle methods?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had actually tried that, but pydocstyle didn't like it since it's not in imperative mood. Now the linter is bossing us around 😆

D401: First line should be in imperative mood; try rephrasing (found 'Callback')

Copy link
Member

Choose a reason for hiding this comment

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

D451: Developer muttered about "burn it all to the ground" which hurt linters feelings; try therapy.

return self.environment.autopush_wait_time(self)

def on_start(self) -> Any:
"""Called when a User starts running."""
"""Call when a websocket starts running."""
Copy link
Contributor

Choose a reason for hiding this comment

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

It is User here, not websocket.

Suggested change
"""Call when a websocket starts running."""
"""Call when a User starts running."""

self.ws_greenlet = gevent.spawn(self.connect)

def on_stop(self) -> Any:
"""Called when a User stops running."""
"""Call when a websocket stops running."""
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
"""Call when a websocket stops running."""
"""Call when a User stops running."""

@taddes taddes requested a review from Trinaa February 1, 2024 15:11
Copy link
Member

@jrconlin jrconlin left a comment

Choose a reason for hiding this comment

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

Looks good!
I took the liberty of suggesting some improved docstrings for some of the tests that actually explain what the test is attempting. I figure that might help both QA and dev define some of the requirements that Autopush has.

I also tried to make sure that we refer to VAPID in upper case in doc strings, because it's technically an acronym and not just something that's lacking taste and dull.

tests/integration/test_integration_all_rust.py Outdated Show resolved Hide resolved
tests/integration/test_integration_all_rust.py Outdated Show resolved Hide resolved
tests/integration/test_integration_all_rust.py Outdated Show resolved Hide resolved
tests/integration/test_integration_all_rust.py Outdated Show resolved Hide resolved
tests/integration/test_integration_all_rust.py Outdated Show resolved Hide resolved
tests/integration/test_integration_all_rust.py Outdated Show resolved Hide resolved
tests/integration/test_integration_all_rust.py Outdated Show resolved Hide resolved
tests/integration/test_integration_all_rust.py Outdated Show resolved Hide resolved
tests/integration/test_integration_all_rust.py Outdated Show resolved Hide resolved
tests/load/locustfiles/locustfile.py Show resolved Hide resolved
@taddes taddes force-pushed the SYNC-3835-pydocstyle-implementation branch from 8716f81 to dbee993 Compare February 4, 2024 03:08
@taddes taddes requested a review from jrconlin February 5, 2024 17:52
@taddes taddes merged commit aa5773d into master Feb 5, 2024
1 check passed
@taddes taddes deleted the SYNC-3835-pydocstyle-implementation branch February 5, 2024 19:28
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