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

Quarkus CLI update test #1868

Merged
merged 1 commit into from
Aug 15, 2024
Merged

Quarkus CLI update test #1868

merged 1 commit into from
Aug 15, 2024

Conversation

mocenas
Copy link
Contributor

@mocenas mocenas commented Jul 2, 2024

Summary

This PR does not work, as it requires quarkus-qe/quarkus-test-framework#1188 to be effective here.

This makes a proposal how Quarkus CLI test could look in the future. Idea is to create common tests and other support methods in parent class and making some kind of a "framework". For each new tested version we can add new class, which should be more or else simple.

Edit: This PR requires quarkus-qe/quarkus-test-framework#1236 and other things in framework 1.5.0.Beta13. So do not merge until this is released and updated in TS.

Please select the relevant options.

  • Bug fix (non-breaking change which fixes an issue)
  • Dependency update
  • Refactoring
  • Backport
  • New scenario (non-breaking change which adds functionality)
  • This change requires a documentation update
  • This change requires execution against OCP (use run tests phrase in comment)

Checklist:

  • Methods and classes used in PR scenarios are meaningful
  • Commits are well encapsulated and follow the best practices

@mocenas mocenas requested review from michalvavrik and gtroitsk July 2, 2024 13:49
@michalvavrik michalvavrik marked this pull request as draft July 3, 2024 07:21
@michalvavrik
Copy link
Member

run tests

@michalvavrik
Copy link
Member

please ignore run tests comment, I just need to test OCP CI and there was no better PR atm.

@mocenas
Copy link
Contributor Author

mocenas commented Aug 2, 2024

I've added actual tests. @gtroitsk Please have a look at them.

@gtroitsk
Copy link
Member

gtroitsk commented Aug 2, 2024

I've added actual tests. @gtroitsk Please have a look at them.

I will look at it later today

Copy link
Member

@gtroitsk gtroitsk left a comment

Choose a reason for hiding this comment

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

I afraid that actual TC is not enough. I think we need to test wider set of changes, scope is up to you. Maybe Michal V. or Rostislav have some comments.
For testing properties ideally we should create real app reflecting the customer's usage scenario including test coverage (taken from TP). We can't relate that new names are syntactically correct.
Also can you please add an option/tests for RHBQ apps.

@mocenas
Copy link
Contributor Author

mocenas commented Aug 5, 2024

I afraid that actual TC is not enough. I think we need to test wider set of changes, scope is up to you. Maybe Michal V. or Rostislav have some comments. For testing properties ideally we should create real app reflecting the customer's usage scenario including test coverage (taken from TP). We can't relate that new names are syntactically correct. Also can you please add an option/tests for RHBQ apps.

I agree that we should tests entire apps. I'm working on it (created first app for hibernate update). I plan to add more for methods and packages renames.

Could you be more specific what would you like to see as TC "wider set of changes"? I'm following recipes and migration guides and testing stuff from them.

Yes I will add RBHQ updates.

@gtroitsk
Copy link
Member

gtroitsk commented Aug 6, 2024

I afraid that actual TC is not enough. I think we need to test wider set of changes, scope is up to you. Maybe Michal V. or Rostislav have some comments. For testing properties ideally we should create real app reflecting the customer's usage scenario including test coverage (taken from TP). We can't relate that new names are syntactically correct. Also can you please add an option/tests for RHBQ apps.

I agree that we should tests entire apps. I'm working on it (created first app for hibernate update). I plan to add more for methods and packages renames.

Could you be more specific what would you like to see as TC "wider set of changes"? I'm following recipes and migration guides and testing stuff from them.

Yes I will add RBHQ updates.

Under TC "wider set of changes" I meant test as must as possible recipes and changes from migration guides. You follow it, so I am fine.

@mocenas
Copy link
Contributor Author

mocenas commented Aug 6, 2024

Testing quarkus udpate on RHBQ has a problem that I didn't forsee. Updating between two RHBQ versions requires to have both their repos available and (AFAIK) in single local maven repo. This doesn't fit our workflow for quarkus-test-suite and IMHO it is not worth it to change the workflow.

I think it would be better to drop this PR and move update tests into separate GH repo, for which we can setup appropriate workflow. WDYT @gtroitsk @rsvoboda @michalvavrik ?

@michalvavrik
Copy link
Member

Testing quarkus udpate on RHBQ has a problem that I didn't forsee. Updating between two RHBQ versions requires to have both their repos available and (AFAIK) in single local maven repo. This doesn't fit our workflow for quarkus-test-suite and IMHO it is not worth it to change the workflow.

If that's a true it's a problem. I'll be honesty, I don't understand why updating from released RHBQ to RHBQ that is being released would required the released RHBQ locally. I'd expect that update command use remote Maven repository. Can you clarify this a bit?

@quarkus-qe quarkus-qe deleted a comment from michalvavrik Aug 7, 2024
@mocenas
Copy link
Contributor Author

mocenas commented Aug 7, 2024

After updating local quarkus CLI setup to work with RHBQ, current test design work surprisingly well with RHBQ.

@mocenas
Copy link
Contributor Author

mocenas commented Aug 7, 2024

I've added support for testing updates to specific bom version.

@mocenas
Copy link
Contributor Author

mocenas commented Aug 9, 2024

Can you please review this @gtroitsk @michalvavrik ? The tests should be done, I'm keeping it as a draft before framework is released.
PS: I know it seems that a lot of migration changes are not covered, but these are usually new feature/deprecated/removed/breaking change/etc, which means how have to manually update you app anyway, so I don't see way in testing it.

Copy link
Member

@michalvavrik michalvavrik left a comment

Choose a reason for hiding this comment

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

I skimmed through changes, they LGTM. I am afraid I don't have proper time to in depth review in the next week and half. AFAICT it's ok. Maybe you will need additional reviewer. Thanks!

@mocenas mocenas force-pushed the quarkus_update branch 2 times, most recently from 23ebf94 to cade3d0 Compare August 15, 2024 09:26
@mocenas mocenas marked this pull request as ready for review August 15, 2024 09:28
@mocenas mocenas changed the title DRAFT: Quarkus CLI update test Quarkus CLI update test Aug 15, 2024
@michalvavrik
Copy link
Member

@mocenas 8 commits for 2 new files is bit excessive, please squash your commits.

@mocenas
Copy link
Contributor Author

mocenas commented Aug 15, 2024

@mocenas 8 commits for 2 new files is bit excessive, please squash your commits.

I intended to squash them during merge, but OK done.

Copy link
Member

@michalvavrik michalvavrik left a comment

Choose a reason for hiding this comment

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

LGTM

@mocenas mocenas mentioned this pull request Aug 15, 2024
9 tasks
@mocenas
Copy link
Contributor Author

mocenas commented Aug 15, 2024

Failures are not related, so I'm merging this.

@mocenas mocenas merged commit 7048d02 into quarkus-qe:main Aug 15, 2024
7 of 14 checks passed
@mocenas mocenas deleted the quarkus_update branch August 15, 2024 13:05
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