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

🌱 Initial Hub API Tests #268

Merged
merged 44 commits into from
May 4, 2023
Merged

🌱 Initial Hub API Tests #268

merged 44 commits into from
May 4, 2023

Conversation

aufi
Copy link
Member

@aufi aufi commented Mar 17, 2023

First part of Hub REST API test suite.

Steps

  • the simplest CRUD for all resource types
  • simple seeds tests
  • connecting resources with refs (update tests above)
  • clarify non-trivial fixtures definition and tear up&down
  • basic integration scenarios (addons&analysis)

Related to #262

@aufi aufi changed the title [WIP] Hub API Tests 🌱 [WIP] Hub API Tests Mar 17, 2023
@aufi
Copy link
Member Author

aufi commented Mar 20, 2023

The PR is related to: #262

test/api/tag/fixtures.go Outdated Show resolved Hide resolved
test/api/client/client.go Outdated Show resolved Hide resolved
test/api/client/client.go Outdated Show resolved Hide resolved
addon/adapter.go Outdated Show resolved Hide resolved
test/api/client/client.go Outdated Show resolved Hide resolved
@aufi
Copy link
Member Author

aufi commented Mar 28, 2023

A sample with RichClient (not fully working) to consider, but it goes slightly away from the API interaction.
aufi@5178977

Update 2022-04-06: Added rich client method together with client.Should and Must for error checking&tests failures control: 625b777

@aufi
Copy link
Member Author

aufi commented Mar 28, 2023

@jortel @mansam @djzager @dymurray Hey! This is a draft for Hub API tests (that might serve building blocks for integration components and QE tests), we had few discussion with Jeff already, but I'd like ask you for review and your thoughts. Thanks!

We can setup a call to discuss it if needed.

Copy link
Contributor

@dymurray dymurray left a comment

Choose a reason for hiding this comment

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

My suggestion:

Samples() function as it exists is okay, since you are returning an array of applications to be used for tests. However, it feels like we are close to table driven testing without actually doing it. I think a small tweak to each test can make it table-driven without too major of a change. You should be defining a set of tests at the beginning of each function, and if you want to pull from the samples to define an application for the test that is good.

See: https://github.com/konveyor/go-konveyor-tests/blob/main/developer/applications-inventory/applications_analyze_test.go#L14 how we define the test cases at the top in a table, and this can give you flexibility of defining test cases where you expect failures and things to go wrong rather than having to write a whole separate test function for it. What do you think?

@aufi
Copy link
Member Author

aufi commented Mar 30, 2023

My suggestion:

Samples() function as it exists is okay, since you are returning an array of applications to be used for tests. However, it feels like we are close to table driven testing without actually doing it. I think a small tweak to each test can make it table-driven without too major of a change. You should be defining a set of tests at the beginning of each function, and if you want to pull from the samples to define an application for the test that is good.

See: https://github.com/konveyor/go-konveyor-tests/blob/main/developer/applications-inventory/applications_analyze_test.go#L14 how we define the test cases at the top in a table, and this can give you flexibility of defining test cases where you expect failures and things to go wrong rather than having to write a whole separate test function for it. What do you think?

Thanks for the comment! Actually even this PR was started with the TestCases and shouldFail true/false structure (e.g. 82d9c32 or ae4d8d0). However it turned out that it doesn't have right fit for our (mostly API multi-step) tests use-case, so there are some modifications.

(WIP - going to add examples and objectives why did it in such way&updating this PR).

@aufi aufi changed the title 🌱 [WIP] Hub API Tests 🌱 Initial Hub API Tests Apr 6, 2023
@aufi
Copy link
Member Author

aufi commented Apr 6, 2023

Added richclient methods, README and sample API integration (slow) test doing an application analysis d49f50d#diff-fcd2bf711a00447192da2d46171f15d0bb6302397b523e0ba92ac9f61bbb8ff7R17-R88.

There are still few things open like update API tests with more table-driven instead of "samples-driven" CRUD tests (together with considering use Samples map instead of array). But as an initial version of Hub tests, I think this PR is ready for review.

@aufi aufi marked this pull request as ready for review April 6, 2023 13:26
@aufi aufi requested a review from sshveta April 6, 2023 17:05
addon/adapter.go Outdated Show resolved Hide resolved
test/api/client/client.go Outdated Show resolved Hide resolved
test/api/client/client.go Outdated Show resolved Hide resolved
@aufi aufi marked this pull request as ready for review May 4, 2023 09:48
@aufi
Copy link
Member Author

aufi commented May 4, 2023

The PR was updated based on discussion from call we had last week with @dymurray @jortel @djzager @mansam and clarification from Jeff this week. The PR is up for review.

I'm about submitting follow-up PRs focusing on

  • covering most of resources in basic API test with the binding package update
  • expansion of Application analysis integration test

Once this PR gets in as a foundation for Konveyor upstream tests, I'm happy discuss other close-future tasks on tests as well as running these tests on PRs/nightly.

Copy link
Contributor

@jortel jortel left a comment

Choose a reason for hiding this comment

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

LGTM
Thanks for all of the hard work on this!

//
// Test application analysis
// "Basic" means that there no other dependencies than the application itself (no need prepare credentials, proxy, etc)
func TestBasicAnalysis(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be in the hub.

Copy link
Member Author

Choose a reason for hiding this comment

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

I partially agree, but having this in test/integration directory (instead of test/api), expecting its move to different repo (or at least be just the integration test proving that Hub can triger addon analysis) and having more advanced analysis test in different repos looked OK-ish for me. Also it could be a helpful example for other&QE how to use the test methods from Hub.
Fot that reason I put it to this PR. If this would block this PR from getting in, I can remove it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Created an issue related to this: #310

Copy link
Member

@djzager djzager left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for all the work on this.

@aufi
Copy link
Member Author

aufi commented May 4, 2023

Thanks for your feedback! Merging.

@aufi aufi merged commit 122ec48 into konveyor:main May 4, 2023
aufi added a commit to aufi/tackle2-hub that referenced this pull request May 5, 2023
aufi added a commit that referenced this pull request May 6, 2023
Adding identity and proxy methods to binding package. Enabling Hub API
test execution on PRs ```Build Hub / test-api (pull_request)``` (in
disconnected mode).

Follow-up on #268

Signed-off-by: Marek Aufart <[email protected]>
aufi added a commit to aufi/tackle2-hub that referenced this pull request May 16, 2023
Adding Indetity and Proxy resources to API tests. Identity provides
samples with login/password, key and settingss file.

Related to konveyor#268

Signed-off-by: Marek Aufart <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

4 participants