Skip to content

Conversation

@telday
Copy link
Contributor

@telday telday commented Nov 23, 2020

No description provided.

@telday telday force-pushed the integration-tests branch 4 times, most recently from ad81174 to 15774ab Compare November 30, 2020 17:31
@telday telday force-pushed the integration-tests branch 4 times, most recently from 393c871 to 8d21476 Compare December 7, 2020 18:40
@telday telday marked this pull request as ready for review December 7, 2020 18:41
@telday telday requested a review from a team as a code owner December 7, 2020 18:41
@telday telday force-pushed the integration-tests branch from 8d21476 to ca19dbd Compare December 7, 2020 18:43
@telday telday changed the title WIP: Add Integration tests in Python Add Integration tests in Python Dec 7, 2020
BradleyBoutcher
BradleyBoutcher previously approved these changes Dec 9, 2020
Copy link

@BradleyBoutcher BradleyBoutcher left a comment

Choose a reason for hiding this comment

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

Great work, LGTM ✨

Copy link
Contributor

@john-odonnell john-odonnell left a comment

Choose a reason for hiding this comment

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

Just a couple formatting comments, and one conceptual question.

"""
resp = self.api.show_public_keys(self.account, 'Variable', 'one/password')

self.assertIsInstance(resp, str)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not necessarily breaking, but wondering if this assertion actually confirms that the API call worked. Seems a bit too general compared to a lot of the other test cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test is definitely lighter then others. However I check that the return type is a string because this endpoint is supposed to return a plain text body so I am just making sure that the client returns the proper type. These are just integration tests so I didn't want to add any checks that were too complicated in.

@john-odonnell
Copy link
Contributor

Also a rebase, PRs that were open before the GH Action was merged won't be caught.

Also added an api_config file which will get a default configuration for tests.
Should reduce levels of code duplication in the tests
Auto-generated import statements in the test files were causing these
messages. Removed them because we cannot change this
Reduces code duplication for the setUpClass and tearDownClass
methods
@telday telday force-pushed the integration-tests branch from 0695f1e to 628b82a Compare December 9, 2020 21:20
Copy link
Contributor

@john-odonnell john-odonnell left a comment

Choose a reason for hiding this comment

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

LGTM

@john-odonnell john-odonnell merged commit 2d67b34 into main Dec 9, 2020
@john-odonnell john-odonnell deleted the integration-tests branch December 28, 2020 23:34
@telday telday restored the integration-tests branch January 19, 2021 14:20
@john-odonnell john-odonnell deleted the integration-tests branch February 1, 2021 17:08
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.

4 participants