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

Cross-version API Compatibility - Cleanup Test Helper functions #35

Merged
merged 3 commits into from
Apr 18, 2023
Merged

Cross-version API Compatibility - Cleanup Test Helper functions #35

merged 3 commits into from
Apr 18, 2023

Conversation

mpanchajanya
Copy link
Contributor

@mpanchajanya mpanchajanya commented Mar 22, 2023 •
edited

What this PR does / why we need it

  • Refactored the tests using updated test helpers.
  • Added individual API test helpers for Servers and Context APIs to be reuse in test cases.

Which issue(s) this PR fixes

Fixes #

Describe testing done for PR

  • Run Unit tests for test/compatibility
  • Build and Run compatibility tests using below command and make sure all tests pass
    make compatibility-tests
    Screenshot 2023-03-22 at 23 31 22

More details on test output can be found from the Github workflow that is run on this PR https://github.com/vmware-tanzu/tanzu-plugin-runtime/actions/runs/4495795430/jobs/7909811037?pr=26

For more details on Cross-version API compatibility testing refer to https://github.com/vmware-tanzu/tanzu-plugin-runtime/blob/main/test/compatibility/docs/cross-version-api-compatibility.md

Release note

Additional information

Special notes for your reviewer

@mpanchajanya mpanchajanya requested a review from a team as a code owner March 22, 2023 04:00
@vuil
Copy link
Contributor

vuil commented Mar 22, 2023

There are multiple references across PRs about the 3 make targets, apparently copied from
test/compatibility/docs/cross-version-api-compatibility.md

If one needs to run the compatibility tests, do we need to run all 3? Seems the first covers the the other 2, no?
So I suggest updating test/compatibility/docs/cross-version-api-compatibility.md to clearly state what one needs to run to execute the compatibility tests. You can still reference the other targets, but as sub headers or children of the main one.

Likewise in future PR. If make compatibility-tests is all that is needed to exercise the tests, just mention it, to reduce noise for the reviewers.

@vuil
Copy link
Contributor

vuil commented Mar 22, 2023

Some linter issues to address.

@mpanchajanya
Copy link
Contributor Author

There are multiple references across PRs about the 3 make targets, apparently copied from test/compatibility/docs/cross-version-api-compatibility.md

If one needs to run the compatibility tests, do we need to run all 3? Seems the first covers the the other 2, no? So I suggest updating test/compatibility/docs/cross-version-api-compatibility.md to clearly state what one needs to run to execute the compatibility tests. You can still reference the other targets, but as sub headers or children of the main one.

Likewise in future PR. If make compatibility-tests is all that is needed to exercise the tests, just mention it, to reduce noise for the reviewers.

You only need one Command make compatibility-tests. Updated the description and doc accordingly

@vuil
Copy link
Contributor

vuil commented Mar 22, 2023

This PR appears to be just refactoring, but I also noticed there are changes in the test result output?
You might want to explain what is happening there as well.
I also suggest updating the PR commits into logical commits with short details to explain the changes.
I would like to review how the commit message actually ends up at time of merge.

@mpanchajanya
Copy link
Contributor Author

Some linter issues to address.

Lints are addressed in #26

@mpanchajanya mpanchajanya force-pushed the cleanup-compatibility-tests branch 4 times, most recently from ff53bdb to 6aadaed Compare March 24, 2023 17:06
vuil
vuil reviewed Apr 11, 2023
View reviewed changes
test/compatibility/core/api_constants.go
@@ -63,7 +63,6 @@ const (
Version0254 RuntimeVersion = "v0.25.4"
Version0280 RuntimeVersion = "v0.28.0"
VersionLatest RuntimeVersion = "latest"
Version100 RuntimeVersion = "v1.0.0"
Copy link
Contributor

@vuil vuil Mar 22, 2023 •
edited

Choose a reason for hiding this comment

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

Version100 still referenced in markdown file. (Forgot to click send a while back, but I think still applies)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed all references for Version100. we will add this when we add tests for 1.0

@mpanchajanya mpanchajanya requested a review from vuil April 18, 2023 04:04
vuil
vuil approved these changes Apr 18, 2023
View reviewed changes
Copy link
Contributor

@vuil vuil 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 the updates

@mpanchajanya mpanchajanya merged commit 9ea858f into vmware-tanzu:main Apr 18, 2023
3 checks passed
vuil pushed a commit that referenced this pull request May 2, 2023
@mpanchajanya @vuil
Cross-version API Compatibility - Cleanup Test Helper functions (#35)
ecec66f 
* Refactor test helper functions to create and resuse api commands, input and output options

* Remove Version100 references

* Update Workflow to run on all files
@marckhouzam marckhouzam added this to the v0.90.0 milestone Oct 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vuil vuil vuil approved these changes

Assignees
No one assigned
Labels
cla-not-required
Projects
None yet
Milestone
v0.90.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@mpanchajanya @vuil @marckhouzam @vmwclabot