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.

Sign up for GitHub

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

Jump to bottom

conformance validation: add new helper to testutil #4532

Merged
merged 3 commits into from
Jun 12, 2020
Merged

conformance validation: add new helper to testutil #4532

merged 3 commits into from
Jun 12, 2020
+33 −0

Conversation

mayankshah1607
Copy link
Contributor

@mayankshah1607 mayankshah1607 commented Jun 2, 2020 •
edited
Loading

Fixes #4530

  • Add new struct GenericTestHelperOptions

GenericTestHelperOptions defines the TestHelper options
that are used along with NewGenericTestHelper() method to obtain a *TestHelper

  • Add new helper function NewGenericTestHelper

NewGenericTestHelper accepts a *GenericTestHelperOptions and returns a new *TestHelper from the options provided. This helper was created to be able to reuse the testutil package without any hard restrictions

Signed-off-by: Mayank Shah mayankshah1614@gmail.com

@mayankshah1607
Add new helper to testutil
6596cd9 
- Add new struct `ConformanceTestHelperOptions`

`ConformanceTestHelperOptions` defines the TestHelper
options required for conformance validation - https://github.com/linkerd/linkerd2-conformance
This way the default options may be defined in the linkerd2-conformance repo,
and an instance can be passed to NewConformanceTestHelper() to obtain a *TestHelper

- Add new helper function `NewConformanceTestHelper`

`NewConformanceTestHelper` accepts a *ConformanceTestHelperOptions and
returns a new *TestHelper from the options provided. This helper was
created to be able to reuse the `testutil` package for Conformance validation

See - linkerd#4530

Signed-off-by: Mayank Shah <mayankshah1614@gmail.com>
@mayankshah1607 mayankshah1607 changed the title Add new helper to testutil conformance validation: add new helper to testutil Jun 2, 2020
@mayankshah1607 mayankshah1607 mentioned this pull request Jun 3, 2020
Closed
Pothulapati
Pothulapati reviewed Jun 4, 2020
View reviewed changes
Copy link
Contributor

@Pothulapati Pothulapati left a comment

Choose a reason for hiding this comment

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

@mayankshah1607 This is even useful for things like #4448 where we would want to initialize a testHelper without hard restrictions, How about having the function name and the options be more generic? It can be used not only with conformance right? :)

@mayankshah1607
Use generic naming conventions
fb071ec 
Signed-off-by: Mayank Shah <mayankshah1614@gmail.com>
@mayankshah1607
Copy link
Contributor Author

@Pothulapati That makes sense! I've updated the names, mind taking a look again? I hope NewGenericTestHelper and GenericTestHelperOptions sound okay :)

Pothulapati
Pothulapati approved these changes Jun 4, 2020
View reviewed changes
Copy link
Contributor

@Pothulapati Pothulapati left a comment

Choose a reason for hiding this comment

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

LGTM!

@mayankshah1607 mayankshah1607 mentioned this pull request Jun 5, 2020
Merged
@mayankshah1607
Copy link
Contributor Author

mayankshah1607 commented Jun 7, 2020 •
edited
Loading

Pinging @alpeb for another approval! :)

@alpeb
Copy link
Member

alpeb commented Jun 8, 2020

@mayankshah1607 can you try embedding structs so that there's not so much duplication between TestHelper and GenericTestHelperOptions? It's ok if you have to make all these fields public...

@mayankshah1607
Update NewGenericTestHelper function signaature
2a5d2f9 
- Instead of depending on a redundant `GenericTestHelperOptions`, the `NewGenericTestHelper` returns
and instance of `TestHelper` from the function arguments provided
- Remove redundant `GenericTestHelperOptions`

Signed-off-by: Mayank Shah <mayankshah1614@gmail.com>
@mayankshah1607
Copy link
Contributor Author

mayankshah1607 commented Jun 9, 2020 •
edited
Loading

@alpeb Making the fields of the TestHelper public would mean refactoring some amount of code as it would render the helper methods (such as GetVersion, GetLinkerdNamespace, etc ) redundant, because the fields may now be directly accessed. If the redundancy of the fields in GenericTestHelperOptions is an issue, how about we remove that struct and instead directly make NewGenericTestHelper return an instance of TestHelper by accepting the field values as function parameters?

This way, the NewGenericTestHelper may be called from anywhere by passing the TestHelper values as function arguments. (Similar to how NewTestHelper accepts flag arguments instead)

This would not only satisfy our current requirements for #4448 and #4530 , but would also ensure that we do not have to deal with refactoring the rest of the integration tests code.

I've just pushed this change (this commit). Could you please have a look and let me know if this sounds okay? Thanks again 😄

Cc: @Pothulapati

@Pothulapati
Copy link
Contributor

Ideally, we would want the fields to be public but we don't see the need for it yet. (especially considering it would take a huge refactor i.e updating fields everywhere).

IMO, we should go with this PR for now, and come back to make the fields public if its needed in the future. WDYT?

@mayankshah1607
Copy link
Contributor Author

@Pothulapati sounds cool!

alpeb
alpeb approved these changes Jun 9, 2020
View reviewed changes
Copy link
Member

@alpeb alpeb left a comment

Choose a reason for hiding this comment

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

Agreed with @Pothulapati , a struct is preferable to a large function signature, but let's not block this PR and we can address that later.

@Pothulapati Pothulapati merged commit 6174b19 into linkerd:master Jun 12, 2020
mayankshah1607 added a commit to mayankshah1607/linkerd2 that referenced this pull request Jun 12, 2020
@mayankshah1607
Follow up PR for linkerd#4532
ad15dca 
- Updates function signature of `NewGenericTestHelper` to accept `version` and an `httpClient`

Signed-off-by: Mayank Shah <mayankshah1614@gmail.com>
@mayankshah1607 mayankshah1607 mentioned this pull request Jun 12, 2020
Merged
mayankshah1607 added a commit to mayankshah1607/linkerd2 that referenced this pull request Jun 18, 2020
@mayankshah1607
Follow up PR for linkerd#4532
56960dc 
- Updates function signature of `NewGenericTestHelper` to accept `version` and an `httpClient`

Signed-off-by: Mayank Shah <mayankshah1614@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alpeb alpeb alpeb approved these changes

@Pothulapati Pothulapati Pothulapati approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

conformance validation: Add new helper method in testutil package
3 participants
@mayankshah1607 @alpeb @Pothulapati